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

[Concurrent Segment Search][Meta] GA readiness items #9100

Closed
sohami opened this issue Aug 3, 2023 · 14 comments · Fixed by #12074
Closed

[Concurrent Segment Search][Meta] GA readiness items #9100

sohami opened this issue Aug 3, 2023 · 14 comments · Fixed by #12074
Assignees
Labels
distributed framework enhancement Enhancement or improvement to existing feature or request v2.12.0 Issues and PRs related to version 2.12.0 v3.0.0 Issues and PRs related to version 3.0.0

Comments

@sohami
Copy link
Collaborator

sohami commented Aug 3, 2023

This is a place holder issue for tracking all the GA readiness work items needed for concurrent search:

  1. Remove the experimental feature flag opensearch.experimental.feature.concurrent_segment_search.enabled
  2. Update the default value of cluster setting search.concurrent_segment_search.enabled to false after above is done
  3. Update tests using feature flag to use cluster setting instead. This will get addressed to large extent by [Concurrent Segment Search] Enable Search ITs with Concurrent Segment Search #7440
  4. Doc updates: a) Profile APIs, b) Cluster/Index Settings, c) New stats that will be added
    [DOC] Concurrent Segment Search support in OpenSearch documentation-website#3662
  5. Remove the log in QueryPhaseSearcherWrapper
  6. Nightly benchmark setup
@sohami sohami added enhancement Enhancement or improvement to existing feature or request distributed framework labels Aug 3, 2023
@sohami sohami removed the untriaged label Aug 3, 2023
@sohami
Copy link
Collaborator Author

sohami commented Sep 7, 2023

@Pallavi-AWS Fyi

@yigithub yigithub added this to 2.11.0 (November 16th, 2023) in OpenSearch Project Roadmap Sep 7, 2023
@sandervandegeijn
Copy link

Maybe a dumb question, why not enable it by default? We have enabled the feature and never saw any disadvantages, even in the experimental stage. Just a decent performance improvement. 👍

@jed326
Copy link
Collaborator

jed326 commented Jan 18, 2024

@sandervandegeijn We definitely want to explore making concurrent segment search the default case but in the first release we want to minimize any potential disruptions to users. We already know that in cases of very high resource usage concurrent search may see some performance regressions. I'm thinking after the GA release of concurrent search we can start a more detailed discussion on the pros/cons of making concurrent search default.

@sandervandegeijn
Copy link

Okay cool :) Caution is almost always a good thing!

@reta
Copy link
Collaborator

reta commented Jan 31, 2024

@sohami @jed326 this is targeting at least 2.13.0 right? (I believe it is super risky for 2.12.0)

@reta reta added v3.0.0 Issues and PRs related to version 3.0.0 v2.13.0 Issues and PRs related to version 2.13.0 labels Jan 31, 2024
@jed326
Copy link
Collaborator

jed326 commented Jan 31, 2024

@sohami @jed326 this is targeting at least 2.13.0 right? (I believe it is super risky for 2.12.0)

@reta We were planning on targetting 2.12.0 still. We haven't made any correctness related changes for a while and haven't uncovered any new issues -- removing the feature flag is all that remains (which I just published the PR for). Did you have any other concerns or risks that you see?

@reta
Copy link
Collaborator

reta commented Jan 31, 2024

We still have a number of test failing [1] for concurrent search path and we don't know the cause of these failure yet, personally I feel this change is risky for 2.12.0 to graduate out of experimental, @CEHENKLE @dblock @nknize @andrross what do you think folks?

[1] https://github.com/opensearch-project/OpenSearch/issues?q=is%3Aissue+is%3Aopen+%22search.concurrent_segment_search.enabled%22%3A%22true%22

@andrross
Copy link
Member

@jed326 Let's go through the search that @reta linked and try to get a resolution to all issues. We shouldn't GA this feature if there are truly unknown test failures remaining, but I'm optimistic that we can address everything by the end of the week.

/cc @yigithub

@jed326
Copy link
Collaborator

jed326 commented Jan 31, 2024

Thanks @reta. I did a quick review of the tests in that search link:

We have been triaging & tracking the failing tests related to concurrent search in the project view here, and as you can see there is only the single test remaining (#11875):
https://github.com/orgs/opensearch-project/projects/117/views/4

@jed326
Copy link
Collaborator

jed326 commented Jan 31, 2024

I did a deeper crawl through all of the open flaky test issues as well and I didn't find any other open issues that are related to concurrent search either.

@reta @andrross Let me know if you think there's anything else we can verify to de-risk a 2.12.0 launch. Thanks!

@reta
Copy link
Collaborator

reta commented Jan 31, 2024

I did a deeper crawl through all of the open flaky test issues as well and I didn't find any other open issues that are related to concurrent search either.

Thanks @jed326 , indeed I see only #11875 to be really related (hopefully we'll get it in soon). Another pending fix is here #10836 but that for tests only.

That definitely makes this change less risky :-) thanks again.

@jed326
Copy link
Collaborator

jed326 commented Jan 31, 2024

That definitely makes this change less risky :-) thanks again.

Thanks @reta! I'll move this issue back to 2.12.0 for now then. As for the GA PR, I will also poke around and see if there are any other maintainers who can take a crack at if while you are busy.

@jed326 jed326 added v2.12.0 Issues and PRs related to version 2.12.0 and removed v2.13.0 Issues and PRs related to version 2.13.0 labels Jan 31, 2024
@andrross
Copy link
Member

andrross commented Feb 1, 2024

I've put together an "operational readiness" issue that compiles all the work and artifacts that went into getting this feature ready for release: #12118

Would love to get feedback on that issue, on both the operational readiness template/process as well as the specific content for the concurrent search release. FYI @reta

@jed326
Copy link
Collaborator

jed326 commented Feb 2, 2024

Going to leave this open until 2.12 is launched

@jed326 jed326 reopened this Feb 2, 2024
@jed326 jed326 removed the untriaged label Feb 6, 2024
@yigithub yigithub closed this as completed Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distributed framework enhancement Enhancement or improvement to existing feature or request v2.12.0 Issues and PRs related to version 2.12.0 v3.0.0 Issues and PRs related to version 3.0.0
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants