Integrates KNN plugin with ConcurrentSearchRequestDecider interface #2111

shatejas · 2024-09-17T01:20:30Z

This allows knn queries to enable concurrency when index.search.concurrent_segment_search.mode or search.concurrent_segment_search.mode in auto mode. Without this the default behavior of auto mode is non-concurrent search

Description

More details in opensearch-project/OpenSearch#15259

Testing

Functional

Manually tested with debugger to make sure ConcurrentQueryPhaseSearcher is used
Unit test and sanity IntegTest added

Performance

Baseline (no settings update)

50th percentile latency,prod-queries,58.86101468404134,ms
90th percentile latency,prod-queries,85.32014465332031,ms
99th percentile latency,prod-queries,95.10733413696289,ms

Concurrent_segment_search.mode: auto

50th percentile latency,prod-queries,45.04893729613377,ms
90th percentile latency,prod-queries,48.14952836545217,ms
99th percentile latency,prod-queries,50.41835594177246,ms

Check List

New functionality includes testing.
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.

navneet1v · 2024-09-17T23:57:58Z

@shatejas for perf runs completeness please add below details

How many shards were used?
What was the dataset used?
What was the machine used?

src/main/java/org/opensearch/knn/plugin/search/KNNConcurrentSearchRequestDecider.java

shatejas · 2024-09-18T00:03:19Z

How many shards were used?

1 shard

What was the dataset used?

cohere 1 million

What was the machine used?

This was run on docker with this configuration
JVM=36g
CPU_COUNT=8
MEM_SIZE=48g

src/main/java/org/opensearch/knn/plugin/search/KNNConcurrentSearchRequestDecider.java

navneet1v · 2024-09-18T00:04:07Z

src/main/java/org/opensearch/knn/plugin/search/KNNConcurrentSearchRequestDecider.java

+        public Optional<ConcurrentSearchRequestDecider> create(IndexSettings indexSettings) {
+            return Optional.of(new KNNConcurrentSearchRequestDecider());
+        }


I think we should not always send the new instance of KNNConcurrentSearchRequestDecider. Only if indexSettings has index.knn = true we should send the new instance. Otherwise even for non k-NN indices this create method will be called, which means that for every querybuilder evaluateForQuery function of KNNConcurrentSearchRequestDecider class.

@shatejas before resolving the comment can you add a comment if you are going to make the change or not?

@navneet1v Resolved after I made the change :)

https://github.com/opensearch-project/k-NN/pull/2111/files#diff-3ecc756cf2b8824250072443c0a8d1953227eccefcfce7216848290d03de81bbR59

navneet1v · 2024-09-18T00:05:25Z

JVM=36g

Curious on why JVM was 36g?

shatejas · 2024-09-18T00:56:35Z

JVM=36g

Curious on why JVM was 36g?

Didn't think memory made a difference as there is no change in amount of threads (as of 2.17) allocated by OS when concurrent segment search code path is used, so gave sufficient memory

src/test/java/org/opensearch/knn/integ/search/ConcurrentSegmentSearchIT.java

navneet1v · 2024-09-18T05:05:16Z

src/test/java/org/opensearch/knn/integ/search/ConcurrentSegmentSearchIT.java

+    }
+
+    @SneakyThrows
+    public void testConcurrentSegmentSearch() {


better to use the convention testABC_whenLMN_thenXYZ()

navneet1v · 2024-09-18T05:05:51Z

src/test/java/org/opensearch/knn/integ/search/ConcurrentSegmentSearchIT.java

+      }
+     */
+    @SneakyThrows
+    private XContentBuilder createFaissHnswIndexMapping(String fieldName, int dimension) {


You can do something like this:

createKnnIndex(testIndex, getKNNDefaultIndexSettings(), createKnnIndexMapping(TEST_FIELD, DIMENSIONS));

I would suggest checking KNNRestTestCase.java class as it has a lot of helper functions to create the index and not create another function in ITs for creating the mappings.

You can do something like this

The mapping is being asserted so I need to separate it out

I would suggest checking KNNRestTestCase.java class as it has a lot of helper functions to create the index and not create another function in ITs for creating the mappings.

The requests for tests should be ideally localized to tests so we unknowingly don't affect multiple different test scenarios. Switching to KNNJsonIndexMappingsBuilder but I would prefer this mapping to be localized to this test

navneet1v · 2024-09-18T05:06:52Z

src/test/java/org/opensearch/knn/integ/search/ConcurrentSegmentSearchIT.java

+        updateIndexSettings(indexName, Settings.builder().put("index.search.concurrent_segment_search.mode", "auto"));
+
+        // Test search queries
+        int k = 10;
+        verifySearch(indexName, fieldName, k);
+
+        updateIndexSettings(indexName, Settings.builder().put("index.search.concurrent_segment_search.mode", "all"));


the indexsetting should be reverted back to default otherwise for all other tests CSS will start to work.

Its using a specific index name for the IT to make sure no other tests are affected.

Will delete the index to be safe

navneet1v · 2024-09-18T05:14:31Z

src/test/java/org/opensearch/knn/plugin/search/KNNConcurrentSearchRequestDeciderTests.java

+
+public class KNNConcurrentSearchRequestDeciderTests extends KNNTestCase {
+
+    public void testDecider() {


same as above use testABC_whenLMN_thenXYZ() for all the test functions

src/main/java/org/opensearch/knn/plugin/search/KNNConcurrentSearchRequestDecider.java

jmazanec15 · 2024-09-18T16:06:05Z

src/main/java/org/opensearch/knn/plugin/search/KNNConcurrentSearchRequestDecider.java

+
+    @Override
+    public void evaluateForQuery(final QueryBuilder queryBuilder, final IndexSettings indexSettings) {
+        if (queryBuilder instanceof KNNQueryBuilder && indexSettings.getValue(KNNSettings.IS_KNN_INDEX_SETTING)) {


Will we be able to look into cpu and memory? Is this done in a chain like fashion?

Will we be able to look into cpu and memory

The control is with core and core decides based on CPU and couple other conditions. I don't think they have any immediate plans to give that control to plugin as per the RFC and plugin RFC

Is this done in a chain like fashion?

I don't completely understand the question. There will be multiple deciders based on number of plugins, it will loop through each of them and evaluate the query builder with visitor pattern using QueryBuilder.visit which evaluates and sets a decision. Even if one decider decides NO it will not execute concurrent search. Let me know if that answers your question

jmazanec15

This looks good to me! Thanks!

test is failing

This allows knn queries to enable concurrency when index.search.concurrent_segment_search.mode or search.concurrent_segment_search.mode in auto mode. Without this the default behavior of auto mode is non-concurrent search Signed-off-by: Tejas Shah <[email protected]>

opensearch-trigger-bot · 2024-09-18T23:10:06Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2111-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0421cdc907b43e4a930bd5a51454e5efea8413b6
# Push it to GitHub
git push --set-upstream origin backport/backport-2111-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2111-to-2.x.

…pensearch-project#2111) This allows knn queries to enable concurrency when index.search.concurrent_segment_search.mode or search.concurrent_segment_search.mode in auto mode. Without this the default behavior of auto mode is non-concurrent search Signed-off-by: Tejas Shah <[email protected]> (cherry picked from commit 0421cdc)

opensearch-trigger-bot · 2024-09-19T17:07:05Z

The backport to 2.17 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.17 2.17
# Navigate to the new working tree
cd .worktrees/backport-2.17
# Create a new branch
git switch --create backport/backport-2111-to-2.17
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0421cdc907b43e4a930bd5a51454e5efea8413b6
# Push it to GitHub
git push --set-upstream origin backport/backport-2111-to-2.17
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.17

Then, create a pull request where the base branch is 2.17 and the compare/head branch is backport/backport-2111-to-2.17.

…pensearch-project#2111) This allows knn queries to enable concurrency when index.search.concurrent_segment_search.mode or search.concurrent_segment_search.mode in auto mode. Without this the default behavior of auto mode is non-concurrent search Signed-off-by: Tejas Shah <[email protected]> (cherry picked from commit 0421cdc)

…2111) (#2132) This allows knn queries to enable concurrency when index.search.concurrent_segment_search.mode or search.concurrent_segment_search.mode in auto mode. Without this the default behavior of auto mode is non-concurrent search Signed-off-by: Tejas Shah <[email protected]> (cherry picked from commit 0421cdc)

…2111) (#2126) This allows knn queries to enable concurrency when index.search.concurrent_segment_search.mode or search.concurrent_segment_search.mode in auto mode. Without this the default behavior of auto mode is non-concurrent search Signed-off-by: Tejas Shah <[email protected]> (cherry picked from commit 0421cdc)

shatejas force-pushed the concurrent-segment-search branch 7 times, most recently from 726db9a to 641620c Compare September 17, 2024 20:45

shatejas marked this pull request as ready for review September 17, 2024 22:41

shatejas requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski, ryanbogan and luyuncheng as code owners September 17, 2024 22:41

navneet1v reviewed Sep 17, 2024

View reviewed changes

src/main/java/org/opensearch/knn/plugin/search/KNNConcurrentSearchRequestDecider.java Outdated Show resolved Hide resolved

navneet1v requested changes Sep 18, 2024

View reviewed changes

shatejas force-pushed the concurrent-segment-search branch from cc1ebaa to 132894c Compare September 18, 2024 00:42

shatejas force-pushed the concurrent-segment-search branch from 132894c to c0b4743 Compare September 18, 2024 00:57

shatejas requested a review from navneet1v September 18, 2024 01:03

shatejas force-pushed the concurrent-segment-search branch 2 times, most recently from eb0c997 to 886d57e Compare September 18, 2024 01:19

navneet1v reviewed Sep 18, 2024

View reviewed changes

jmazanec15 reviewed Sep 18, 2024

View reviewed changes

shatejas force-pushed the concurrent-segment-search branch from 886d57e to 11a1d0e Compare September 18, 2024 19:03

navneet1v self-requested a review September 18, 2024 19:16

shatejas requested a review from jmazanec15 September 18, 2024 19:20

jmazanec15 previously approved these changes Sep 18, 2024

View reviewed changes

shatejas force-pushed the concurrent-segment-search branch from e502d6f to 921e5d7 Compare September 18, 2024 21:24

navneet1v added Enhancements Increases software capabilities beyond original client specifications backport 2.x labels Sep 18, 2024

navneet1v approved these changes Sep 18, 2024

View reviewed changes

jmazanec15 approved these changes Sep 18, 2024

View reviewed changes

junqiu-lei merged commit 0421cdc into opensearch-project:main Sep 18, 2024
39 of 41 checks passed

shatejas mentioned this pull request Sep 19, 2024

[Backport 2.x] Integrates KNN plugin with ConcurrentSearchRequestDecider interface (… #2126

Merged

5 tasks

navneet1v added the backport 2.17 label Sep 19, 2024

shatejas mentioned this pull request Sep 19, 2024

[Backport 2.17] Integrates KNN plugin with ConcurrentSearchRequestDecider interface (… #2132

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrates KNN plugin with ConcurrentSearchRequestDecider interface #2111

Integrates KNN plugin with ConcurrentSearchRequestDecider interface #2111

shatejas commented Sep 17, 2024 •

edited

Loading

navneet1v commented Sep 17, 2024 •

edited

Loading

shatejas commented Sep 18, 2024 •

edited

Loading

navneet1v Sep 18, 2024

navneet1v Sep 18, 2024

shatejas Sep 18, 2024

navneet1v commented Sep 18, 2024

shatejas commented Sep 18, 2024

navneet1v Sep 18, 2024

navneet1v Sep 18, 2024

shatejas Sep 18, 2024

navneet1v Sep 18, 2024

shatejas Sep 18, 2024

navneet1v Sep 18, 2024

navneet1v Sep 18, 2024

jmazanec15 Sep 18, 2024

shatejas Sep 18, 2024

jmazanec15 left a comment

opensearch-trigger-bot bot commented Sep 18, 2024

opensearch-trigger-bot bot commented Sep 19, 2024


		public class KNNConcurrentSearchRequestDeciderTests extends KNNTestCase {

		public void testDecider() {

Integrates KNN plugin with ConcurrentSearchRequestDecider interface #2111

Integrates KNN plugin with ConcurrentSearchRequestDecider interface #2111

Conversation

shatejas commented Sep 17, 2024 • edited Loading

Description

Testing

Functional

Performance

Check List

navneet1v commented Sep 17, 2024 • edited Loading

shatejas commented Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navneet1v commented Sep 18, 2024

shatejas commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmazanec15 left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Sep 18, 2024

opensearch-trigger-bot bot commented Sep 19, 2024

shatejas commented Sep 17, 2024 •

edited

Loading

navneet1v commented Sep 17, 2024 •

edited

Loading

shatejas commented Sep 18, 2024 •

edited

Loading