Doc update for concurrent search #8181
Conversation
|
Thank you for submitting your PR. The PR states are In progress (or Draft) -> Tech review -> Doc review -> Editorial review -> Merged. Before you submit your PR for doc review, make sure the content is technically accurate. If you need help finding a tech reviewer, tag a maintainer. When you're ready for doc review, tag the assignee of this PR. The doc reviewer may push edits to the PR directly or leave comments and editorial suggestions for you to address (let us know in a comment if you have a preference). The doc reviewer will arrange for an editorial review. |
Gankris96
requested review from
AMoo-Miki,
Naarcha-AWS,
dlvenable,
epugh,
kolchfa-aws,
natebower,
stephen-crawford and
vagimeli
as code owners
|
@kolchfa-aws / @Naarcha-AWS can you please take a look |
|
@Gankris96 Thank you, will do! |
Gankris96
force-pushed
the
main
branch
4 times, most recently
from
8d61a34 to
be7c9b2
Compare
Signed-off-by: Ganesh Ramadurai <[email protected]>
kolchfa-aws
added
the
release-notes
There was a problem hiding this comment.
Thank you, @Gankris96! Please see my comments and suggestions and let me know if you have any questions. The Developer information section was first added when concurrent segment search was experimental. Now, I think it's better if we move it to the GitHub repo docs and link it from here. The doc site contains user documentation. @sohami Let me know what you think.
|
@Gankris96 Also, is this feature experimental for 2.17? |
It's not experimental. Users will be allowed to change the setting and directly start using this feature. There is no specific feature_flag as such that they need to update to be able to use this, if that's what you're asking. |
Do you mean, we should remove it from the documentation website? What do you mean by GitHub repo docs? You mean like under |
|
@Gankris96 Yes, have it as part of (probably) DEVELOPER_GUIDE on GitHub directly in the repo. This is where developers will look for it, generally. Our site only contains information about using OpenSearch, not extending it (or anything that relates to its internal structure). |
@kolchfa-aws This probably deserves a separate issue for deeper discussion. The information we're talking about here under Coming at it from a different direction, when we say using concurrent segment search, one way is interacting through APIs and enabling cluster settings, but another way of using the feature is, as a plugin developer, knowing which of these interfaces you need to implement in order to use the feature. |
|
@jed326 Agreed, we should have more documentation for plugin developers. However, I think, developing a plugin does not fall under using OpenSearch. That's why this section seems out of place here. We should have this documentation in the repo. For example, the Developer Guide in the opensearch-sdk-java repo has information about building your own extension. |
Co-authored-by: kolchfa-aws <[email protected]> Signed-off-by: Ganesh Krishna Ramadurai <[email protected]>
There was a problem hiding this comment.
@Gankris96 @kolchfa-aws Please see my comments and changes and let me know if you have any questions. Thanks!
|
|
||
| ### AggregatorFactory changes | ||
|
|
||
| Because of implementation details, not all aggregator types can support concurrent segment search. To accommodate this, we have introduced a [`supportsConcurrentSegmentSearch()`](https://github.com/opensearch-project/OpenSearch/blob/2.x/server/src/main/java/org/opensearch/search/aggregations/AggregatorFactory.java#L123) method in the `AggregatorFactory` class to indicate whether a given aggregation type supports concurrent segment search. By default, this method returns `false`. Any aggregator that needs to support concurrent segment search must override this method in its own factory implementation. |
There was a problem hiding this comment.
"Because of the implementation configuration"?
There was a problem hiding this comment.
This is actually existing wording: https://opensearch.org/docs/latest/search-plugins/concurrent-segment-search/#developer-information-aggregatorfactory-changes
There was a problem hiding this comment.
IMO we probably don't need to make changes to this paragraph since it's already published, I think it's just showing up in this PR because it was moved, wdyt @kolchfa-aws @natebower ?
There was a problem hiding this comment.
@jed326 I think "implementation details" reads better because it's commonly used.
Thanks @kolchfa-aws , I'll create a separate issue to follow up on this since I think we should spend a little more time in the core repo writing out search plugin documentation. |
Co-authored-by: Nathan Bower <[email protected]> Signed-off-by: kolchfa-aws <[email protected]>
* Doc update for concurrent search Signed-off-by: Ganesh Ramadurai <[email protected]> * Apply suggestions from code review Co-authored-by: kolchfa-aws <[email protected]> Signed-off-by: Ganesh Krishna Ramadurai <[email protected]> * Apply suggestions from code review Co-authored-by: Nathan Bower <[email protected]> Signed-off-by: kolchfa-aws <[email protected]> --------- Signed-off-by: Ganesh Ramadurai <[email protected]> Signed-off-by: Ganesh Krishna Ramadurai <[email protected]> Signed-off-by: kolchfa-aws <[email protected]> Co-authored-by: kolchfa-aws <[email protected]> Co-authored-by: Nathan Bower <[email protected]> Signed-off-by: Eric Pugh <[email protected]>
5 participants
Description
Adds documentation for concurrent search
Issues Resolved
Resolves #8068
Version
2.17
Frontend features
If you're submitting documentation for an OpenSearch Dashboards feature, add a video that shows how a user will interact with the UI step by step. A voiceover is optional.
Checklist
For more information on following Developer Certificate of Origin and signing off your commits, please check here.