HttpRemoteTaskRunner: Fix markLazyWorkers for maxLazyWorkers == 0.#14532
Merged
gianm merged 1 commit intoapache:masterfrom Jul 6, 2023
Merged
HttpRemoteTaskRunner: Fix markLazyWorkers for maxLazyWorkers == 0.#14532gianm merged 1 commit intoapache:masterfrom
gianm merged 1 commit intoapache:masterfrom
Conversation
kfaraz
reviewed
Jul 6, 2023
| * @param maxLazyWorkers maximum number of lazy workers to return | ||
| */ | ||
| Collection<Worker> markWorkersLazy(Predicate<ImmutableWorkerInfo> isLazyWorker, int maxWorkers); | ||
| Collection<Worker> markWorkersLazy(Predicate<ImmutableWorkerInfo> isLazyWorker, int maxLazyWorkers); |
Contributor
There was a problem hiding this comment.
Nit: This method name is probably not accurate anymore, because it does not mark workers as lazy. It gets the list of workers which are lazy and are safe to terminate. So maybe getLazyWorkersToTerminate?
Suggested change
| Collection<Worker> markWorkersLazy(Predicate<ImmutableWorkerInfo> isLazyWorker, int maxLazyWorkers); | |
| Collection<Worker> getLazyWorkersToTerminate(Predicate<ImmutableWorkerInfo> isLazyWorker, int maxToTerminate); |
Contributor
Author
There was a problem hiding this comment.
I agree the name is funny, but I didn't want to rename it since it's an interface method. I figured for interfaces that people might be using in extensions, best to retain existing signatures unless there is an important functional reason to change them.
kfaraz
approved these changes
Jul 6, 2023
Contributor
kfaraz
left a comment
kfaraz
left a comment
There was a problem hiding this comment.
Change looks good. Minor comment about method name.
Contributor
Author
|
The only failure is due to #14531, which is unrelated. |
gianm
added a commit
to gianm/druid
that referenced
this pull request
Jul 7, 2023
Recently apache#14532 fixed a problem when maxLazyWorkers == 0 and lazyWorkers starts out empty. Unfortunately, even after that patch, there remained a more general version of this problem when maxLazyWorkers == lazyWorkers.size(). This patch fixes it. I'm not sure if this would actually happen in production, because the provisioning strategies do try to avoid calling markWorkersLazy until previously-initiated terminations have finished. Nevertheless, it still seems like a good thing to fix.
gianm
added a commit
that referenced
this pull request
Jul 7, 2023
…4545) Recently #14532 fixed a problem when maxLazyWorkers == 0 and lazyWorkers starts out empty. Unfortunately, even after that patch, there remained a more general version of this problem when maxLazyWorkers == lazyWorkers.size(). This patch fixes it. I'm not sure if this would actually happen in production, because the provisioning strategies do try to avoid calling markWorkersLazy until previously-initiated terminations have finished. Nevertheless, it still seems like a good thing to fix.
sergioferragut
pushed a commit
to sergioferragut/druid
that referenced
this pull request
Jul 21, 2023
sergioferragut
pushed a commit
to sergioferragut/druid
that referenced
this pull request
Jul 21, 2023
…ache#14545) Recently apache#14532 fixed a problem when maxLazyWorkers == 0 and lazyWorkers starts out empty. Unfortunately, even after that patch, there remained a more general version of this problem when maxLazyWorkers == lazyWorkers.size(). This patch fixes it. I'm not sure if this would actually happen in production, because the provisioning strategies do try to avoid calling markWorkersLazy until previously-initiated terminations have finished. Nevertheless, it still seems like a good thing to fix.
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
3 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.
RemoteTaskRunneralready handles this. Use a similar approach inHttpRemoteTaskRunner.