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

Improves after_job_end hook #409

Closed
wants to merge 3 commits into from
Closed

Improves after_job_end hook #409

wants to merge 3 commits into from

Conversation

AngellusMortis
Copy link
Contributor

Since finish_job can fail, I adjusted the after_job_end hook to always receive a copy of the the JobResult that would get written into redis.

I also updated all of the other places that recorded a failing job to call after_job_end as well and added a asyncio.shield to after_job_end as it is a great way to do something with the job result (like generate metrics for New Relic/Prometheus/etc.).

@AngellusMortis AngellusMortis marked this pull request as draft July 5, 2023 18:48
@AngellusMortis AngellusMortis marked this pull request as ready for review July 5, 2023 19:00
@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #409 (6859e3e) into main (ab2dda2) will decrease coverage by 0.09%.
The diff coverage is 93.75%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #409      +/-   ##
==========================================
- Coverage   98.40%   98.31%   -0.09%     
==========================================
  Files          11       11              
  Lines        1063     1071       +8     
  Branches      200      201       +1     
==========================================
+ Hits         1046     1053       +7     
  Misses          8        8              
- Partials        9       10       +1     
Files Coverage Δ
arq/worker.py 98.40% <93.75%> (-0.18%) ⬇️

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...6859e3e. Read the comment docs.

@gerazenobi
Copy link

@AngellusMortis hi there 👋

I am taking advantage of your deeper knowledge regarding this hook.

question: without this PR the hook after_job_end only works when there were no errors and jobs successfully finished?
E.g. if the job raised an exception, the hook won't be called ?

If that is the case, would there be a work around using other hooks ?

Thanks in advance

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.

In general looks good, mostly just needs some docstrings.

Comment on lines +489 to +490
async def job_failed(exc: BaseException, ref: Optional[str] = None) -> None:
ref = ref or f'{job_id}:{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
async def job_failed(exc: BaseException, ref: Optional[str] = None) -> None:
ref = ref or f'{job_id}:{function_name}'
async def job_failed(exc: BaseException, ref: str = f'{job_id}:{function_name}') -> None:

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

async def _after_job_end(self, ctx: Dict[Any, Any], job_result: Optional[JobResult]) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

please add a docstring.

queue_name: str,
*,
serializer: Optional[Serializer] = None,
) -> Tuple[JobResult, Optional[bytes]]:
Copy link
Member

Choose a reason for hiding this comment

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

please add a docstring explaining what this does.

@AngellusMortis
Copy link
Contributor Author

I no longer care.

@AngellusMortis AngellusMortis deleted the improve-after-job branch April 2, 2024 03:26
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.

3 participants