-
-
Notifications
You must be signed in to change notification settings - Fork 731
Enable Client.wait_for_workers to optionally also wait when removing workers #6377
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Enable Client.wait_for_workers to optionally also wait when removing workers #6377
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this feature overall makes sense but I'm wondering about the API. The behavior so far was basically a min
and what you're proposing now is an equal
but maybe it should be a max
? (I don't have a strong opinion, just asking the question)
Thanks for the review @fjetter. Yeah, I also thought about Ideally, I would love to allow users to set either For something as small as this I didn't really want to go through any deprecations or breaks. One alternative could be to have a Also thanks for the tip about status, I'll have a look. |
My preference would be a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me. Thanks for adding this functionality, I've just stumbled across a use case for this.
Co-authored-by: Hendrik Makait <hendrik.makait@gmail.com>
…t-wait-workers-absolute
Thanks for the review feedback @gjoseph92, @hendrikmakait, @ian-r-rose. I've tweaked the PR, but this is my first time switching out an |
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 15 files ± 0 15 suites ±0 6h 28m 36s ⏱️ - 5m 31s For more details on these failures, see this check. Results for commit 4b2b946. ± Comparison against base commit 4f6960a. ♻️ This comment has been updated with latest results. |
Co-authored-by: Gabe Joseph <gjoseph92@gmail.com> Co-authored-by: Lawrence Mitchell <wence@gmx.li>
Appreciate the review here @gjoseph92 @wence-! Addressed everything except making the condition check more DRY, I'm really struggling to think of a clean way to write this. Originally it was lambdas which were more readable, but |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jacobtomlinson!
…t-wait-workers-absolute
Test failures are unrelated so I intend to merge this in 24 hours if there are no further comments. |
@jacobtomlinson are you going to merge this? |
Thanks for the nudge @gjoseph92. @wence- made a suggestion on making it more readable so I've applied that and am waiting on CI again. Should be good to merge as long as no related tests fail. |
Closes #6374
pre-commit run --all-files
Adds an
absolute
kwarg toClient.wait_for_workers
that makes it wait for exactlyn_workers
instead of the current behaviour of waiting for at leastn_workers
. Default isFalse
to continue with current behaviour.No strong feelings about the kwarg name here, I chose
absolute
but something likeexact
would be fine too.