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

Introduce new setting search.concurrent.max_slice to control the slice computation for concurrent segment search #8884

Merged
merged 3 commits into from
Jul 28, 2023

Conversation

sohami
Copy link
Collaborator

@sohami sohami commented Jul 26, 2023

Description

Introduces a static setting in 2.x to control slice computation using lucene default or max target slice mechanism for concurrent segment search. This is replacement of PR #8847

Related Issues

Resolves #7358

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
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.search.SearchWeightedRoutingIT.testStrictWeightedRoutingWithCustomString

@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Merging #8884 (a63eb0d) into 2.x (5164fe2) will increase coverage by 0.22%.
The diff coverage is 87.09%.

@@             Coverage Diff              @@
##                2.x    #8884      +/-   ##
============================================
+ Coverage     70.64%   70.87%   +0.22%     
- Complexity    57265    57400     +135     
============================================
  Files          4745     4747       +2     
  Lines        271070   271099      +29     
  Branches      39984    39986       +2     
============================================
+ Hits         191508   192132     +624     
+ Misses        63130    62476     -654     
- Partials      16432    16491      +59     
Files Changed Coverage Δ
...org/opensearch/search/SearchBootstrapSettings.java 62.50% <62.50%> (ø)
...search/search/internal/MaxTargetSliceSupplier.java 92.30% <92.30%> (ø)
...rg/opensearch/common/settings/ClusterSettings.java 93.18% <100.00%> (ø)
server/src/main/java/org/opensearch/node/Node.java 86.06% <100.00%> (+0.01%) ⬆️
...ensearch/search/internal/ContextIndexSearcher.java 66.10% <100.00%> (+1.39%) ⬆️

... and 464 files with indirect coverage changes

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      3 org.opensearch.search.SearchWeightedRoutingIT.testStrictWeightedRoutingWithCustomString

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.search.SearchWeightedRoutingIT.testStrictWeightedRoutingWithCustomString
      1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testPitCreatedOnReplica
      1 org.opensearch.indices.replication.SegmentReplicationTargetServiceTests.testShardAlreadyReplicating
      1 org.opensearch.index.shard.RemoteStoreRefreshListenerTests.testRefreshSuccessAfterFailureInFirstAttemptAfterSnapshotAndMetadataUpload

@sohami
Copy link
Collaborator Author

sohami commented Jul 26, 2023

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.search.SearchWeightedRoutingIT.testStrictWeightedRoutingWithCustomString
      1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testPitCreatedOnReplica
      1 org.opensearch.indices.replication.SegmentReplicationTargetServiceTests.testShardAlreadyReplicating
      1 org.opensearch.index.shard.RemoteStoreRefreshListenerTests.testRefreshSuccessAfterFailureInFirstAttemptAfterSnapshotAndMetadataUpload

unrelated test failures

@sohami
Copy link
Collaborator Author

sohami commented Jul 27, 2023

We would also need to have a documentation update for this new setting, could you please create an issue at https://github.com/opensearch-project/documentation-website, thank you.

@reta Concurrent search is an experimental feature currently so I was planning to track this as part of documentation with GA. Don't think we will need to document it now as it cannot be used standalone without enabling the feature

@sohami
Copy link
Collaborator Author

sohami commented Jul 27, 2023

@reta Have addressed other review comments. Thanks!

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@sohami
Copy link
Collaborator Author

sohami commented Jul 27, 2023

Gradle Check (Jenkins) Run Completed with:

known flaky tests #6090

@reta
Copy link
Collaborator

reta commented Jul 27, 2023

@reta Concurrent search is an experimental feature currently so I was planning to track this as part of documentation with GA

Sure, your pick (documenting experimental features like that would let people to try them out before GA, possibility filing bugs and issues)

@reta
Copy link
Collaborator

reta commented Jul 27, 2023

@reta Have addressed other review comments. Thanks!

Thanks @sohami , one small comment (missed that but you pointed that out) but LGTM otherwise

@sohami
Copy link
Collaborator Author

sohami commented Jul 27, 2023

@reta Have addressed other review comments. Thanks!

Thanks @sohami , one small comment (missed that but you pointed that out) but LGTM otherwise

@reta updated and rebased.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@sohami
Copy link
Collaborator Author

sohami commented Jul 27, 2023

@reta Have addressed other review comments. Thanks!

Thanks @sohami , one small comment (missed that but you pointed that out) but LGTM otherwise

@reta updated and rebased.

oops there are some test failures with latest changes. looking into it

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.search.aggregations.bucket.DateRangeIT.classMethod
      1 org.opensearch.search.aggregations.bucket.DateHistogramOffsetIT.classMethod
      1 org.opensearch.search.SearchWeightedRoutingIT.testStrictWeightedRoutingWithCustomString
      1 org.opensearch.action.bulk.BulkProcessorIT.testBulkProcessorConcurrentRequestsReadOnlyIndex

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@reta
Copy link
Collaborator

reta commented Jul 27, 2023

Still needs some adjustments, failing tests

sohami added 3 commits July 27, 2023 18:18
…e computation for concurrent

segment search. It uses lucene default mechanism if the setting value is <=0 otherwise uses custom max target slice mechanism

Signed-off-by: Sorabh Hamirwasia <[email protected]>
Signed-off-by: Sorabh Hamirwasia <[email protected]>
Signed-off-by: Sorabh Hamirwasia <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@sohami
Copy link
Collaborator Author

sohami commented Jul 28, 2023

Still needs some adjustments, failing tests

fixed now.

@reta
Copy link
Collaborator

reta commented Jul 28, 2023

@andrross since you have neen looking into this change for main, any concerns / comments? thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants