Add resizable write/search queue to OpenSearch by rguo-aws · Pull Request #826 · opensearch-project/OpenSearch

rguo-aws · 2021-06-08T00:01:38Z

Signed-off-by: Ruizhen ruizhen@amazon.com

Description

Create a new type of threadpool queue "OpenSearchResizableBlockingQueue" to dynamically adjust search queue size in runtime.
The current threadpool queue for search is of "FIXED" type and the queue capacity can only be updated via opensearch.yml.

This OpenSearchResizableBlockingQueue that is introduced in this PR can allow us to make changes to queue size without restarting OpsearchSearch process.

Also addressed unit test failures in this PR

Issues Resolved

#476

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

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.

Signed-off-by: Ruizhen <ruizhen@amazon.com>

opensearch-ci-bot · 2021-06-08T00:02:47Z

✅ DCO Check Passed 507b3c7

opensearch-ci-bot · 2021-06-08T00:03:56Z

✅ Gradle Wrapper Validation success 507b3c7

opensearch-ci-bot · 2021-06-08T00:13:46Z

✅ Gradle Precommit success 507b3c7

harold-wang · 2021-06-08T01:43:16Z

start gradle check

opensearch-ci-bot · 2021-06-08T02:08:52Z

❌ Gradle Check failure 507b3c7
Log 227

Reports 227

AmiStrn · 2021-06-14T17:13:06Z

I think the commit message should have a bit more content describing what was done, I have seen quite a few commits to main recently that have either a messy changelog or no message at all.
I will open an issue regarding this in the forums and link here, but this is a current PR that maybe can be fixed:)
A good reference is https://dhwthompson.com/2019/my-favourite-git-commit

AmiStrn · 2021-06-14T17:51:31Z

server/src/main/java/org/opensearch/threadpool/ThreadPool.java

                // 5.x doesn't know about the "fixed_auto_queue_size" thread pool type, just write fixed.
                out.writeString(ThreadPoolType.FIXED.getType());
+            } else if (type == ThreadPoolType.RESIZABLE) {
+                // old vfi doesn't know about "resizable" thread pool. Convert RESIZABLE to FIXED


Can you please use the full term instead of the acronym vfi? acronyms are sometimes a burden for developers to feel comfortable with the project, especially when they are in a comment.

Thanks for pointing this out, I rephrased my comments and removed "vfi" from it

AmiStrn · 2021-06-14T18:02:26Z

server/src/test/java/org/opensearch/search/query/QueryPhaseTests.java

+    //TODO: The executor of our new SiFi resizable search queue is OpenSearchThreadPoolExecutor which
+    // unfortunately does not keep track of queueSize/EWMA execution time. We should ideally Add EWMA enabled threadpool
+    // executor and reenable the below unit test
+    // EWMATrackingEsThreadPoolExecutor from ES 8.0 and re-enable the below unit test.


I don't think anything should be taken from ES 8.0 as it is not Apache License Version 2.0.
I would definitely rephrase this comment and refrain from writing things that would lead anyone to believe (wrongfully) that the code is "from" anywhere but your own original design 😬

Sorry for the confusion. We do not have intent to port code from ES 8.0. It was an error in comments and I've already cleared it up

AmiStrn · 2021-06-14T18:12:41Z

server/src/main/java/org/opensearch/common/util/concurrent/OpenSearchExecutors.java

+    public static OpenSearchThreadPoolExecutor newResizable(String name, int size, int queueCapacity,
+                                                    ThreadFactory threadFactory, ThreadContext contextHolder) {
+        BlockingQueue<Runnable> queue;
+        if (queueCapacity < 0) {


does it make any sense to be ok with a queue size of 0?

yes, Queue size can be set to -1 to make it unbounded. This is to handle the case where queue size is -1 in opensearch.yml

I am asking specifically about 0 though. I get the need for a -1 (although a bit confusing) as a flag for unbounded. I am asking to understand more than any form of critique of course:)

Bukhtawar · 2021-06-16T05:20:18Z

server/src/main/java/org/opensearch/common/util/concurrent/OpenSearchExecutors.java

+        if (queueCapacity < 0) {
+            queue = ConcurrentCollections.newBlockingQueue();
+        } else {
+            queue = new SifiResizableBlockingQueue<>(ConcurrentCollections.<Runnable>newBlockingQueue(),


Suggested change

queue = new SifiResizableBlockingQueue<>(ConcurrentCollections.<Runnable>newBlockingQueue(),

queue = new OpenSearchResizableBlockingQueue<>(ConcurrentCollections.<Runnable>newBlockingQueue(),

thanks. made the changed as per your suggestion

Bukhtawar · 2021-06-16T05:21:58Z

server/src/main/java/org/opensearch/threadpool/ThreadPool.java

        map.put(Names.ANALYZE, ThreadPoolType.FIXED);
-        map.put(Names.WRITE, ThreadPoolType.FIXED);
-        map.put(Names.SEARCH, ThreadPoolType.FIXED_AUTO_QUEUE_SIZE);
+        map.put(Names.WRITE, ThreadPoolType.RESIZABLE);


Do we really need to change WRITE threadpool as they have been moved to support byte-size based defaulting to 10k

that is a very good point. yes, there is actually no need to change write to resizable anymore

Bukhtawar · 2021-06-16T05:23:13Z

test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java

+        // Note: Disable this, since search pool has been changed to be of type RESIZABLE, which does not support a
+        // min_queue_size setting.
+        /*
        if (rarely()) {
            // Sometimes adjust the minimum search thread pool size, causing
            // QueueResizingOpenSearchThreadPoolExecutor to be used instead of a regular
            // fixed thread pool
            builder.put("thread_pool.search.min_queue_size", 100);
        }
+         */


Remove rather than comment

Bukhtawar · 2021-06-16T05:23:54Z

server/src/main/java/org/opensearch/common/util/concurrent/SifiResizableBlockingQueue.java

+ * Mainly makes sense to use with blocking queues that are unbounded to provide the ability to do
+ * capacity verification.
+ */
+public class SifiResizableBlockingQueue<E> extends SizeBlockingQueue<E> {


Rename better

Signed-off-by: Ruizhen <ruizhen@amazon.com>

opensearch-ci-bot · 2021-07-01T22:50:50Z

✅ DCO Check Passed 96352ea

opensearch-ci-bot · 2021-07-01T22:51:54Z

✅ Gradle Wrapper Validation success 96352ea

opensearch-ci-bot · 2021-07-01T23:01:02Z

✅ Gradle Precommit success 96352ea

opensearch-ci-bot · 2021-07-02T22:15:26Z

✅ DCO Check Passed 96352ea

opensearch-ci-bot · 2021-07-02T22:15:42Z

✅ Gradle Wrapper Validation success 96352ea

opensearch-ci-bot · 2021-07-02T22:43:32Z

✅ Gradle Precommit success 96352ea

reta · 2021-08-23T16:59:46Z

...er/src/main/java/org/opensearch/common/util/concurrent/OpenSearchResizableBlockingQueue.java

+ * Mainly makes sense to use with blocking queues that are unbounded to provide the ability to do
+ * capacity verification.
+ */
+public class OpenSearchResizableBlockingQueue<E> extends SizeBlockingQueue<E> {


What are the reasons to introduce this class instead of using the existing ResizableBlockingQueue ?

reta · 2021-08-23T16:59:54Z

server/src/main/java/org/opensearch/common/util/concurrent/SizeBlockingQueue.java


    private final BlockingQueue<E> queue;
-    private final int capacity;
+    protected volatile int capacity;


I think this change should not be taken lightly: the truly fixed size queues (SizeBlockingQueue) would have to pay a price of accessing the volatile field which is only adjustable for resizeable queues.

opensearch-ci-bot · 2021-09-29T21:21:31Z

Can one of the admins verify this patch?

anasalkouz · 2021-10-12T18:49:59Z

@rguo-aws are you working on this? there are couple of comments need to be addressed.

opensearch-ci-bot · 2021-10-26T15:04:40Z

✅ Gradle Wrapper Validation success 96352ea

opensearch-ci-bot · 2021-10-26T15:06:16Z

✅ DCO Check Passed 96352ea

opensearch-ci-bot · 2021-10-26T15:41:11Z

✅ Gradle Precommit success 96352ea

dblock · 2022-03-03T19:33:38Z

@AmiStrn or @reta any interest in picking this up from where @rguo-aws left it at?
Otherwise @anasalkouz, let's find an owner to finish this?

AmiStrn · 2022-03-03T21:49:51Z

Thanks @dblock but Im not available for this in this quarter, so I kindly decline.

reta · 2022-03-03T21:55:13Z

@dblock I am in, will take a look shortly

CEHENKLE · 2022-05-04T17:17:07Z

Hey @reta ! Are you still thinking about this one? Thanks!

reta · 2022-05-04T17:24:45Z

@CEHENKLE yes! I will pick up shortly, thanks for reminding !

CEHENKLE · 2022-05-04T19:57:04Z

@reta Cool beans -- thanks! :)

reta · 2022-05-05T20:28:32Z

@CEHENKLE continuing the work in https://github.com/opensearch-project/OpenSearch/pull/3207/files since I cannot push changes to the original branch.

reta · 2022-05-06T16:51:30Z

Closing in favor of #3207

* Make search/write queue resizable Signed-off-by: Ruizhen <ruizhen@amazon.com> * Address PR comments Signed-off-by: Ruizhen <ruizhen@amazon.com> * Refactoring resizable queue implementation Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Addressing code review comments Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Addressing code review comments Signed-off-by: Andriy Redko <andriy.redko@aiven.io> * Addressing code review comments Signed-off-by: Andriy Redko <andriy.redko@aiven.io> Co-authored-by: Ruizhen <ruizhen@amazon.com>

Make search/write queue resizable

507b3c7

Signed-off-by: Ruizhen <ruizhen@amazon.com>

AmiStrn reviewed Jun 14, 2021

View reviewed changes

Bukhtawar reviewed Jun 16, 2021

View reviewed changes

nknize added enhancement Enhancement or improvement to existing feature or request v1.1.0 Issues, PRs, related to the 1.1.0 release v2.0.0 Version 2.0.0 labels Jun 25, 2021

Address PR comments

96352ea

Signed-off-by: Ruizhen <ruizhen@amazon.com>

dblock mentioned this pull request Jul 16, 2021

Create a new type of threadpool queue "SifiResizableBlockingQueue" to dynamically adjust write/search queue size in runtime. #476

Closed

reta reviewed Aug 23, 2021

View reviewed changes

anasalkouz removed the v1.1.0 Issues, PRs, related to the 1.1.0 release label Sep 7, 2021

tlfeng added v3.0.0 Issues and PRs related to version 3.0.0 and removed v2.0.0 Version 2.0.0 labels Apr 2, 2022

tlfeng removed the v3.0.0 Issues and PRs related to version 3.0.0 label Apr 11, 2022

reta mentioned this pull request May 5, 2022

Add resizable search queue to OpenSearch (picking up #826) #3207

Merged

5 tasks

reta closed this May 6, 2022

	queue = new SifiResizableBlockingQueue<>(ConcurrentCollections.<Runnable>newBlockingQueue(),
	queue = new OpenSearchResizableBlockingQueue<>(ConcurrentCollections.<Runnable>newBlockingQueue(),

Conversation

rguo-aws commented Jun 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

opensearch-ci-bot commented Jun 8, 2021

Uh oh!

opensearch-ci-bot commented Jun 8, 2021

Uh oh!

opensearch-ci-bot commented Jun 8, 2021

Uh oh!

harold-wang commented Jun 8, 2021

Uh oh!

opensearch-ci-bot commented Jun 8, 2021

Uh oh!

AmiStrn commented Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmiStrn Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmiStrn Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opensearch-ci-bot commented Jul 1, 2021

Uh oh!

opensearch-ci-bot commented Jul 1, 2021

Uh oh!

opensearch-ci-bot commented Jul 1, 2021

Uh oh!

opensearch-ci-bot commented Jul 2, 2021

Uh oh!

opensearch-ci-bot commented Jul 2, 2021

Uh oh!

opensearch-ci-bot commented Jul 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opensearch-ci-bot commented Sep 29, 2021

Uh oh!

anasalkouz commented Oct 12, 2021

Uh oh!

opensearch-ci-bot commented Oct 26, 2021

Uh oh!

opensearch-ci-bot commented Oct 26, 2021

Uh oh!

opensearch-ci-bot commented Oct 26, 2021

Uh oh!

rguo-aws commented Jun 8, 2021 •

edited

Loading

AmiStrn commented Jun 14, 2021 •

edited

Loading

AmiStrn Jun 14, 2021 •

edited

Loading

AmiStrn Jun 14, 2021 •

edited

Loading

AmiStrn commented Mar 3, 2022 •

edited

Loading