Skip to content

Add setup and teardown callbacks#14

Merged
sheharyarn merged 1 commit intosheharyarn:masterfrom
danxexe:feature/job-setup-and-teardown
Apr 24, 2019
Merged

Add setup and teardown callbacks#14
sheharyarn merged 1 commit intosheharyarn:masterfrom
danxexe:feature/job-setup-and-teardown

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