Skip to content

Propagate worker address and improve _remove_from_processing behavior#6946

Merged
gjoseph92 merged 5 commits intodask:mainfrom
hendrikmakait:propagate-worker-information
Aug 24, 2022
Merged

Propagate worker address and improve _remove_from_processing behavior#6946
gjoseph92 merged 5 commits intodask:mainfrom
hendrikmakait:propagate-worker-information

Conversation

@hendrikmakait
Copy link
Copy Markdown
Member

@hendrikmakait hendrikmakait commented Aug 24, 2022

This PR follows some online and offline discussions with @crusaderky and @gjoseph92 around the desired functionality of _remove_from_processing and how we best set ts.erred_on to the address of the worker that a task erred on without ambiguity.

We make the following changes:
We generally want to avoid returning a stale worker (address) whenever suitable. Since _remove_from_processing performs the check already, we want to return None when the worker turns out to be stale. We further want to avoid sending around worker addresses since they're crappy IDs (#6392). Finally, we decided to require the worker argument in transition_processing_erred to always be able to refer back to the original worker the task erred on.

See #6939, https://github.com/dask/distributed/pull/6884/files#r952990650, and https://github.com/dask/distributed/pull/6884/files#r945761680 for more context.

  • Tests added / passed
  • Passes pre-commit run --all-files

Copy link
Copy Markdown
Collaborator

@gjoseph92 gjoseph92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I like this approach. Let's see what CI thinks.

Co-authored-by: Gabe Joseph <gjoseph92@gmail.com>
@github-actions
Copy link
Copy Markdown
Contributor

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       15 files  ±0         15 suites  ±0   6h 30m 12s ⏱️ - 15m 43s
  3 041 tests ±0    2 952 ✔️  - 1    83 💤 ±0  6 +1 
22 493 runs  ±0  21 514 ✔️  - 1  973 💤 ±0  6 +1 

For more details on these failures, see this check.

Results for commit 441380e. ± Comparison against base commit 599708e.

@hendrikmakait hendrikmakait marked this pull request as ready for review August 24, 2022 19:11
@gjoseph92
Copy link
Copy Markdown
Collaborator

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