Skip to content
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

Integrates KNN plugin with ConcurrentSearchRequestDecider interface #2111

Merged

Conversation

shatejas
Copy link
Collaborator

@shatejas shatejas commented Sep 17, 2024

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
Copy link
Collaborator

navneet1v commented Sep 17, 2024

@shatejas for perf runs completeness please add below details

  1. How many shards were used?
  2. What was the dataset used?
  3. What was the machine used?

@shatejas
Copy link
Collaborator Author

shatejas commented Sep 18, 2024

  • 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

Comment on lines 54 to 63
public Optional<ConcurrentSearchRequestDecider> create(IndexSettings indexSettings) {
return Optional.of(new KNNConcurrentSearchRequestDecider());
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@navneet1v
Copy link
Collaborator

JVM=36g

Curious on why JVM was 36g?

@shatejas
Copy link
Collaborator Author

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

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

@SneakyThrows
public void testConcurrentSegmentSearch() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to use the convention testABC_whenLMN_thenXYZ()

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +78 to +79
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"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Will delete the index to be safe

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure.


public class KNNConcurrentSearchRequestDeciderTests extends KNNTestCase {

public void testDecider() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
jmazanec15 previously approved these changes Sep 18, 2024
Copy link
Member

@jmazanec15 jmazanec15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Thanks!

@jmazanec15 jmazanec15 dismissed their stale review September 18, 2024 19:34

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]>
@navneet1v navneet1v added Enhancements Increases software capabilities beyond original client specifications backport 2.x labels Sep 18, 2024
@junqiu-lei junqiu-lei merged commit 0421cdc into opensearch-project:main Sep 18, 2024
39 of 41 checks passed
@opensearch-trigger-bot
Copy link
Contributor

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.

shatejas added a commit to shatejas/k-NN that referenced this pull request Sep 19, 2024
…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
Copy link
Contributor

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.

shatejas added a commit to shatejas/k-NN that referenced this pull request Sep 19, 2024
…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)
shatejas added a commit to shatejas/k-NN that referenced this pull request Sep 19, 2024
…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)
shatejas added a commit to shatejas/k-NN that referenced this pull request Sep 19, 2024
…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)
shatejas added a commit to shatejas/k-NN that referenced this pull request Sep 19, 2024
…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)
junqiu-lei pushed a commit that referenced this pull request Sep 20, 2024
…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)
junqiu-lei pushed a commit that referenced this pull request Sep 20, 2024
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport 2.17 Enhancements Increases software capabilities beyond original client specifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants