Merged
Conversation
6eee403 to
26d67c7
Compare
Member
Author
|
The deadlock itself should be fixed by #5525 I recommend merging this anyhow since it reduces complexity |
3 tasks
jrbourbeau
reviewed
Nov 18, 2021
Member
jrbourbeau
left a comment
There was a problem hiding this comment.
Offline @fjetter mentioned he is confident in the changes here and would be good to include them in the upcoming release (xref dask/community#206). To that end, I'm planning to merge this PR in a bit if there are no further comments
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 might fix #5482
This is mostly a refactoring around the release key implementation (which is historically easily top3 contributor for deadlocks). It is now more streamlined and all release and propagate forgotten logic is now pulled together into
transition_generic_releasedandtransition_released_forgottenwhere it should be.Worker.release_keynow basically merely performs a state reset, as it is intended. Transition logic and recommendation should be as compact as possible to make reasoning more easy.I also removed a few places were we set a recommendation to "forgotten" without actual reason. The new logic is now more stable and should fix #5482 and goes as
If a Task is released and does not have any dependents it is allowed to be forgotten
It's hard to tell since it's been spread across the state machine but the previous condition was much more loosely defined.
I'll try reproducing #5482 and write a test for it...