Add resizable search queue to OpenSearch (picking up #826) #3207
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
Signed-off-by: Ruizhen <ruizhen@amazon.com>
Signed-off-by: Ruizhen <ruizhen@amazon.com>
reta
changed the title
|
❌ Gradle Check failure 5a2545794f668c9c2fe95b24f8586cac2c81f620 |
|
❌ Gradle Check failure 15ff5f07270ce4fa52e2d295e983758233f05b4f |
|
start gradle check |
|
❌ Gradle Check failure 15ff5f07270ce4fa52e2d295e983758233f05b4f |
|
start gradle check |
|
❌ Gradle Check failure 15ff5f07270ce4fa52e2d295e983758233f05b4f |
Bukhtawar
reviewed
May 6, 2022
|
✅ Gradle Check success 5f64ad05fb82d3cfd9deeedc2cc37a8195b88f69 |
|
❌ Gradle Check failure edff7a9965c997939a9860b59fd0b465200bab9a |
|
❌ Gradle Check failure 67df4c756ecc17b6144a8ee07b4156b46e4254f6 |
5 tasks
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
|
❌ Gradle Check failure 2a15da8b7bda031a6f47f7d6fb156b7b3bdb650b |
saratvemulapalli
previously approved these changes
May 6, 2022
...n/java/org/opensearch/common/util/concurrent/QueueResizableOpenSearchThreadPoolExecutor.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
Thanks @dblock !
This PR goes side by side with #2595, we replacing the SEARCH_XXX pools with the ones where the size could be adjusted at runtime. Right now this is not exposed to the outside world through API but plugins could do such adjustments. |
|
start gradle check |
|
start gradle check |
Bukhtawar
reviewed
May 12, 2022
Comment on lines
15
to
17
| // This is a random starting point alpha. TODO: revisit this with actual testing and/or make it configurable | ||
| double EWMA_ALPHA = 0.3; | ||
|
|
There was a problem hiding this comment.
Should this be configurable, maybe more of an expert settings
There was a problem hiding this comment.
This is the default, but I will add the constructors to allow configuration
| ) { | ||
|
|
||
| if (queueCapacity <= 0) { | ||
| throw new IllegalArgumentException("queue capacity for [" + name + "] executor must be positive, got: " + queueCapacity); |
There was a problem hiding this comment.
Maybe consider 0 in the exception
There was a problem hiding this comment.
Sorry, didn't get this one: positive means > 0 and 0 is not acceptable value, makes sense?
Comment on lines
+23
to
+26
| public final class QueueResizableOpenSearchThreadPoolExecutor extends OpenSearchThreadPoolExecutor | ||
| implements | ||
| EWMATrackingThreadPoolExecutor { | ||
|
|
| * Resizes the work queue capacity of the pool | ||
| * @param capacity the new capacity | ||
| */ | ||
| public synchronized int resize(int capacity) { |
There was a problem hiding this comment.
For my understanding who calls resize?
There was a problem hiding this comment.
AFAIK that's could be done from the plugin(s), as per attached issue
There was a problem hiding this comment.
Should the plugin have the capability able to override the resize logic? Do you think we could expose a contract?
There was a problem hiding this comment.
I believe this is the whole purpose of the issue and the change (I am finalizing the #826 since the pull request was abandoned). From own perspective - it could be useful in certain cases since thread pools are not adjustable at runtime.
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
|
start gradle check |
Bukhtawar
approved these changes
May 13, 2022
|
@saratvemulapalli anything left on your side? thank you! |
Thanks @reta No I dont have anything else. |
owaiskazi19
reviewed
May 13, 2022
...n/java/org/opensearch/common/util/concurrent/QueueResizableOpenSearchThreadPoolExecutor.java
Show resolved
Hide resolved
dblock
approved these changes
May 16, 2022
|
@reta Any reason not to backport to 2.x? No breaking changes here AFAIK. |
It changes the pool behind SEARCH and SEARCH_THROTTLED, we thought with @andrross that it is breaking change in #2595. I am about to run the benchmarks to evaluate the impact (if any), we could target |
|
I it's a net performance improvement without any visible backwards incompatible changes, then I don't see why not. Breaking changes are only about user experience, APIs, interfaces. |
Got it, thanks @dblock , I will run benchmarks shortly and update the issue on backport plans, thanks! |
5 tasks
@dblock sorry for delay, finally run the tests I wanted, regarding
There is only one - removal of the deprecated properties for If you think this is a backward compatibility concern, we could still bring the change to 2.1.0 but without changing Regrading performance, no regressions shown by the benchmarks:
|
So, if a user has this in a config, will it break? Or just not use these? It's okay to deprecate settings with warnings (e.g. this setting is no longer used), but users' existing configuration should continue loading. |
Yes
They already are deprecated (and should be gone in 2.0 technically) |
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.
Description
Create a new type of threadpool of "RESIZALE" type to dynamically adjust search queue size in runtime. The current threadpools only be updated via
opensearch.yml. Picking up the work from #826.This PR goes side by side with #2595, we replacing the SEARCH_XXX pools with the ones where the size could be adjusted at runtime. Right now this is not exposed to the outside world through API but plugins could do such adjustments.
Issues Resolved
Closes #476
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.