[Discuss] Move concurrent-search out of sandbox plugin to core under feature flag

[sohami]

Is your feature request related to a problem? Please describe.
Currently concurrent-search is implemented as a sandbox plugin which acts as a provider for 1) QueryPhaseSearcher and 2) Threadpool to run segment level search concurrently. All the logic to integrate or implement Lucene CollectionManager interface across different operators (like MinScore, TopN, EarlyTermination, etc) is part of the core. Plugin provides a way create the corresponding searcher instance to perform search via collection manager. However, I think this should be a core feature instead because of following reasons and wanted to discuss and get community feedback on the same.

• When all the gaps with concurrent search is addressed it can become default mechanism in the core to perform searches on segment either sequentially or concurrently. Until then it can be controlled by a feature flag mechanism in the core to use existing flow or the concurrent search flow. I don't foresee multiple different implementation of concurrent search itself which requires it to be plugin. We can control the behavior of concurrent search via below mechanisms. The last 2 can be exposed in a pluggable way with defaults in core.

  • Cluster setting to always fallback to sequential behavior
  • By having settings to control the slicing logic of IndexSearcher which will determine the concurrency
  • By some other means like considering resource utilization of a node to determine if search should be done concurrently or not

• With support in core all the existing search related tests (specially ITs) covering all the different scenarios for the supported DSL can be reused. This is future proof as any new addition to the search DSL will be tested out of the box. The OpenSearchIntegTestCase and other base class can provide a mechanism to control enabling/disabling search behavior to provide coverage for concurrent and sequential search path as the final result should be same.

• This will also help to have coverage for all the other search related plugins like GeospatialPlugin, etc. Plugins specific feature tests will remain in plugin and the test framework can provide mechanism to enable/disable concurrent search.

Describe the solution you'd like
To move concurrent-search to core behind the feature flag until the feature is complete and ready to be released

Additional context

• While performing the PoC for Concurrent Segment Search Overview and Aggs HLD #6798 was exploring how to test existing and new functionality of concurrent search using IT framework. This doesn't seem possible with current model of having a sandbox plugin
• Users of the community who are interested in using the concurrent search in experimental phase can use feature flag based mechanism to enable/disable it as well with ease. Add support using sandbox functionality in official releases #3677

[sohami]

@reta @nknize @andrross @anasalkouz - Tagging you folks to get feedback on above

[reta]

@sohami Looks like deja vu, the concurrent search was implemented as core feature behind the feature flag but @nknize suggested to move it to sandbox plugin.

While performing the PoC for #6798 was exploring how to test existing and new functionality of concurrent search using IT framework. This doesn't seem possible with current model of having a sandbox plugin

The test should be exercising the AggregationPhase, but I agree that e2e test would need plugin to be present in order to make sure everything works.

[sohami]

@reta Thanks for the background. Yes unit tests are fine but concurrent search is changing the execution model for search request, so e2e tests will be needed to provide better coverage in my opinion. All the functionality is already in core and plugin is gatekeeping the usage of it which is essentially a feature flag behavior.

[nknize]

I have no opposition to promoting it from sandbox back to a first class plugin. We started there because the feature flag mechanism wasn't well vetted at the time nor did we have good javadoc annotation guard rails. The gating mechanisms are pretty well vetted at this point from features like segrep and others. Do we have any usage feedback or benchmarks? I'm pretty confident it's a value add but it would be good to have some updated usage and performance data to substantiate the promotion. This would be a first example of a feature promoting from sandbox. Maybe you can propose a timeline to move from feature flag to GA? What does that look like to you? One minor?

Maybe we get this on the project board and target a GA release?

[reta]

Thanks @nknize

Do we have any usage feedback or benchmarks?

No we don't, we didn't have any progress on publishing sandbox plugins (#3677) so no easy way to use it unless build from source. Limited benchmarks were done and shared on the issue / pull request before (if I am not mistaken).

[reta]

Oh @nknize I think @sohami's suggestion is move away from plugin model to core feature (== server module probably), unless I misunderstood

[anasalkouz]

Until then it can be controlled by a feature flag mechanism in the core to use existing flow or the concurrent search flow

Please make sure to follow the best practices of the feature flag: https://github.com/opensearch-project/.github/blob/main/CONTRIBUTING.md#experimental-features

[sohami]

@reta & @nknize Thanks for your inputs. Yes the proposal is to move to core behind a feature flag. This will also help to develop missing pieces and iterate faster similar to other core features like segrep.

Do we have any usage feedback or benchmarks? I'm pretty confident it's a value add but it would be good to have some updated usage and performance data to substantiate the promotion

I am planning to run the benchmark once we have support for aggs and will share the results. But @reta did shared the initial benchmark with his PR. Ref here

Maybe you can propose a timeline to move from feature flag to GA? What does that look like to you? One minor?
Maybe we get this on the project board and target a GA release?

As far as timeline is considered for GA for now I can target it for June mid/end which probably will map to 3.x based on the project board here. I have added the list of things that needs to be addressed for GA in this issue in addition to what is tracked here.

[sohami]

@nknize @reta: Following up on this to see if there is any concern to move it to the core server module and behind a feature flag ?

[reta]

@nknize @reta: Following up on this to see if there is any concern to move it to the core server module and behind a feature flag ?

No objections from me if there is a real need to do so (as you mentioned, if that helps to unblock other features).