Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use function's keep_results configuration when storing failed jobs results #433

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

joshwilson-dbx
Copy link

@joshwilson-dbx joshwilson-dbx commented Feb 20, 2024

Taking a stab at using a function's (or cron job's) configuration for keeping the job results when the job fails.

I don't think this is a complete solution yet. There are a few cases where the job results are written to redis using the Worker's configuration because we can't get the Function in order to use it's config.

But maybe this is a situation where it's good enough to improve the situation for the most common situation?

Addresses #417
Relates to #416

Copy link

codecov bot commented Feb 20, 2024

Codecov Report

Merging #433 (a06ea04) into main (ab2dda2) will decrease coverage by 0.37%.
The diff coverage is 73.33%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #433      +/-   ##
==========================================
- Coverage   98.40%   98.03%   -0.37%     
==========================================
  Files          11       11              
  Lines        1063     1071       +8     
  Branches      200      205       +5     
==========================================
+ Hits         1046     1050       +4     
- Misses          8       10       +2     
- Partials        9       11       +2     
Files Coverage Δ
arq/worker.py 97.80% <73.33%> (-0.78%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab2dda2...a06ea04. Read the comment docs.

@epicwhale epicwhale mentioned this pull request Mar 19, 2024
Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise I think this looks okay, @JonasKs WDYT?

@@ -483,6 +483,7 @@ async def run_job(self, job_id: str, score: int) -> None: # noqa: C901
abort_job = False

function_name, enqueue_time_ms = '<unknown>', 0
function: Optional[Union[Function, CronJob]] = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function: Optional[Union[Function, CronJob]] = None
function: Union[Function, CronJob, None] = None

Comment on lines +520 to +521
with contextlib.suppress(KeyError):
function = self.functions[function_name]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
with contextlib.suppress(KeyError):
function = self.functions[function_name]
function = self.functions.get(function_name)

@@ -701,7 +703,9 @@ async def finish_job(
tr.delete(*delete_keys) # type: ignore[unused-coroutine]
await tr.execute()

async def finish_failed_job(self, job_id: str, result_data: Optional[bytes]) -> None:
async def finish_failed_job(
self, job_id: str, result_data: Optional[bytes], function: Optional[Union[Function, CronJob]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self, job_id: str, result_data: Optional[bytes], function: Optional[Union[Function, CronJob]]
self, job_id: str, result_data: Optional[bytes], function: Union[Function, CronJob, None]

@JonasKs
Copy link
Collaborator

JonasKs commented Apr 2, 2024

Agree, I think this looks good. 😊

@epicwhale
Copy link

Thanks for looking at this one! Any chance this can enter the v0.26.0b1 release? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants