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

Add setup and teardown callbacks #14

Merged

Conversation

danxexe
Copy link
Contributor

@danxexe danxexe commented Jan 18, 2019

This feature introduces two new worker callbacks, on_setup and on_teardown, that expose the whole job struct, not just its arguments. My intended use case was to add a custom per-job logger backend, but it can be used to run any setup / cleanup code that depends on a job's id, pid, etc.

This PR also fixes a timing dependent test intermittence which happened when a spawned job task took longer than 3ms to finish. This is accomplished by introducing a wait_for_children helper which monitors the spawned tasks.

@danxexe
Copy link
Contributor Author

danxexe commented Apr 23, 2019

Any feedback on this PR?

@sheharyarn sheharyarn merged commit 4c7abbb into sheharyarn:master Apr 24, 2019
@sheharyarn
Copy link
Owner

This looks great @danxexe! Thank you for the PR! 🎉

Once you've rebased #16, I'll update the readme/docs and publish a new release. Thank you again!

@sheharyarn
Copy link
Owner

I've made some minor changes to this and published a new release v0.10.0. The setup/teardown callbacks also only receive the arguments and not the full job struct.

@danxexe
Copy link
Contributor Author

danxexe commented May 6, 2019

@sheharyarn receiving only the arguments in the setup/teardown callbacks defeats the purpose of the PR since it makes those callbacks practically indistinguishable from the current perform/on_success/on_failure callbacks. My use case is a custom logger backend that needs the job's id during setup and teardown and I can see many other use cases that would benefit from having access to the full Job struct (ex: add custom monitors to the job's pid), that's why I decided to introduce the new callbacks.

@sheharyarn
Copy link
Owner

I thought about this more and you're right. I should've discussed this more before making those changes. I've reverted them and pushed a new release with this.

Thanks again!

@danxexe
Copy link
Contributor Author

danxexe commented Jun 3, 2019

Cool, thanks 👍

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.

2 participants