Handle workunits corresponding to canceled Nodes. #10659
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
gshuflin
force-pushed
the
workunits-not-completed
branch
from
cf4989d
to
18ee626
Compare
| WorkunitState::Completed { .. } => "Completed:", | ||
| fn log_workunit_state(&self, canceled: bool) { | ||
| let state = match (&self.state, canceled) { | ||
| (_, true) => "Canceled:", |
There was a problem hiding this comment.
I don't know this part of the codebase very well. Does it make sense to have WorkunitState::Canceled instead?
There was a problem hiding this comment.
We'll eventually probably want to represent workunit failure as part of the state, but adding that variant to the type now means we have to modify code in the pathway to the streaming workunit handler right now. I think we still need to do a little bit of thinking about how we want to present failed workunits to plugins, and that can come as a follow-up commit to this one.
[ci skip-build-wheels]
[ci skip-build-wheels]
gshuflin
force-pushed
the
workunits-not-completed
branch
from
18ee626
to
722724e
Compare
|
sgtm |
asherf
approved these changes
Aug 21, 2020
Eric-Arellano
approved these changes
Aug 23, 2020
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.
Problem
cf. #10650 . When a node is canceled by the engine, the engine will remove the future associated with that node without communicating this fact to the workunit store. This means that the store has no idea that a Started but not yet Completed workunit is never going to complete.
Solution
This commit adds a
CanceledWorkunitGuardin thewith_workunitpublic interface to workunits. If a workunit completes normally, the last thing it will do is callnot_canceled()on the guard, and the guard will do nothing. If the guard is dropped before this happens, though, itsDropimplementation will call a cleanup function on the workunit store to remove the workunit from the heavy hitters and display a log message stating that the workunit has been canceled.