jobs, *: remove job Resumer OnSuccess and OnTerminal #45829
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We can just do whatever it was doing in Resume to simplify state machine. Release note: none.
Whatever it was doing can be done in Resume or OnFailOrCancel, both of which are resumable (and most of these were only doing things on success anyway so they should just be last step of Resume). Release note: none.
|
This change is |
spaskob
approved these changes
Mar 7, 2020
There was a problem hiding this comment.
Awesome! Thanks for tackling this! Please also stress test jobs_test.go.
Reviewed 9 of 9 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @pbardea)
pbardea
approved these changes
Mar 7, 2020
|
303 runs so far, 0 failures, over 5m0s |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This eliminates the OnSuccess and OnTerminal methods from the job Resumer API.
The OnSuccess method is clearly duplicative with Resume: it is called when, and only when, Resume() has returned successfully, so whatever work it was going to do could just be done at the end of Resume instead -- and almost all implementations had already been refactored to do so. There was no need to share a txn with moving the job to the terminal state -- the job itself can use a txn to do whatever work it wants, optionally updating a field in the job progress or details with that same txn if it wants those semantics.
The OnTerminal method is similarly surplus to requirements -- whatever it was doing can simply be done at the end of both Resume and OnFailOrCancel. In practice, most OnTerminals were entirely logic conditional on success, to push rows into the result channel, making it trivial to simply move that code to the end of Resume.
Release note: none.