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

Add early termination support for concurrent segment search #8306

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

jed326
Copy link
Collaborator

@jed326 jed326 commented Jun 28, 2023

Description

This PR adds support for partial results in the cases of timeouts for concurrent segment search.
"Full" partial results are not supported for early terminations because shard failures are expected in that case and thus the behavior between concurrent and non-concurrent search is already consistent. See #817 for more details.

We achieve this by swallowing the timeout exception in the ContextIndexSearcher.searchLeaf method and forces us to finish processing all of the leaves in both the sequential and concurrent cases.

Related Issues

Resolves #2586

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:

@jed326 jed326 force-pushed the concurrent-early-term branch from 917dc38 to 90a37b8 Compare June 28, 2023 00:27
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@jed326 jed326 force-pushed the concurrent-early-term branch from 90a37b8 to 0173f13 Compare June 28, 2023 00:35
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@jed326
Copy link
Collaborator Author

jed326 commented Jun 28, 2023

Screenshot 2023-06-28 at 10 48 56 AM

The Gradle Check failure is for an unrelated test, but I am seeing a performance issue in the timeout case. I don't expect the timeout test to take 4x as long as the no timeout case. I see the same runtime over several runs.

@jed326
Copy link
Collaborator Author

jed326 commented Jun 28, 2023

Upon closer inspection this difference in time is actually due to ingesting 1000 docs for the timeout case vs only 10 for the no timeout case. When using 100 docs for both the time is much more aligned:

Screenshot 2023-06-28 at 11 45 52 AM

I'm going to keep the 1000 docs for the timeout case though so that we can keep the check that we are only getting a portion of the expected docs. With 100 docs and 1ms timeout I am seeing that the test still sometimes does collect all 100 docs before the timeout.

@jed326 jed326 force-pushed the concurrent-early-term branch from 1bc3857 to 819f20f Compare July 7, 2023 18:39
@jed326 jed326 force-pushed the concurrent-early-term branch from 819f20f to bcda9fb Compare July 7, 2023 19:09
@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2023

Gradle Check (Jenkins) Run Completed with:

@jed326
Copy link
Collaborator Author

jed326 commented Jul 7, 2023

Test failures look unrelated but I'll rebase the branch and rerun the tests

@jed326 jed326 force-pushed the concurrent-early-term branch from bcda9fb to 5fa3ba7 Compare July 7, 2023 19:21
@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2023

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      2 org.opensearch.remotestore.SegmentReplicationRemoteStoreIT.classMethod
      1 org.opensearch.remotestore.SegmentReplicationRemoteStoreIT.testPressureServiceStats

@jed326
Copy link
Collaborator Author

jed326 commented Jul 7, 2023

Looks like all the tests pass after rebasing, @reta I think we're good to merge then if no other comments. Thanks!

@reta reta merged commit 31e67c1 into opensearch-project:main Jul 7, 2023
@reta reta added backport 2.x Backport to 2.x branch v3.0.0 Issues and PRs related to version 3.0.0 v2.9.0 'Issues and PRs related to version v2.9.0' labels Jul 7, 2023
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

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

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
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-8306-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 31e67c17ecc6eb70d28e991b13085411e961b5bd
# Push it to GitHub
git push --set-upstream origin backport/backport-8306-to-2.x
# Go back to the original working tree
popd
# 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-8306-to-2.x.

@reta
Copy link
Collaborator

reta commented Jul 7, 2023

@jed326 sadly 2.x needs manual backport, could you please take care of it? thank you

jed326 added a commit to jed326/OpenSearch that referenced this pull request Jul 7, 2023
reta pushed a commit that referenced this pull request Jul 8, 2023
vikasvb90 pushed a commit to raghuvanshraj/OpenSearch that referenced this pull request Jul 12, 2023
raghuvanshraj pushed a commit to raghuvanshraj/OpenSearch that referenced this pull request Jul 12, 2023
dzane17 pushed a commit to dzane17/OpenSearch that referenced this pull request Jul 12, 2023
buddharajusahil pushed a commit to buddharajusahil/OpenSearch that referenced this pull request Jul 18, 2023
baba-devv pushed a commit to baba-devv/OpenSearch that referenced this pull request Jul 29, 2023
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch v2.9.0 'Issues and PRs related to version v2.9.0' v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Concurrent Searching: Support Early Termination
3 participants