-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[Backport 2.x] Initial search pipelines implementation (#6587) #7075
[Backport 2.x] Initial search pipelines implementation (#6587) #7075
Conversation
* Initial search pipelines implementation This commit includes the basic features of search pipelines (see opensearch-project/search-processor#80). Search pipelines are modeled after ingest pipelines and provide a simple, clean API for components to modify search requests and responses. With this commit we can: 1. Can create, retrieve, update, and delete search pipelines. 2. Transform search requests and responses by explicitly referencing a pipeline. Later work will include: 1. Adding an index setting to specify a default search pipeline. 2. Allowing search pipelines to be defined within a search request (for development/testing purposes, akin to simulating an ingest pipeline). 3. Adding a collection of search pipeline processors to support common useful transformations. (Suggestions welcome!) Signed-off-by: Michael Froh <froh@amazon.com> * Incorporate feedback from @reta and @navneet1v 1. SearchPipelinesClient: JavaDoc fix 2. SearchRequest: Check versions when (de)serializing new "pipeline" property. 3. Rename SearchPipelinesPlugin -> SearchPipelinePlugin. 4. Pipeline: Change visibility to package private 5. SearchPipelineProcessingException: New exception type to wrap exceptions thrown when executing a pipeline. Bonus: Added an integration test for filter_query request processor. Signed-off-by: Michael Froh <froh@amazon.com> * Register SearchPipelineProcessingException Also added more useful messages to unit tests to explicitly explain what hoops need to be jumped through in order to add a new serializable exception. Signed-off-by: Michael Froh <froh@amazon.com> * Remove unneeded dependencies from search-pipeline-common I had copied some dependencies from ingest-common, but they are not used by search-pipeline-common (yet). Signed-off-by: Michael Froh <froh@amazon.com> * Avoid cloning SearchRequest if no SearchRequestProcessors Also, add tests to confirm that a pipeline with no processors works fine (as a no-op). Signed-off-by: Michael Froh <froh@amazon.com> * Use NamedWritableRegistry to deserialize SearchRequest Queries are serialized as NamedWritables, so we need to use a NamedWritableRegistry to deserialize. Signed-off-by: Michael Froh <froh@amazon.com> * Check for empty pipeline with CollectionUtils.isEmpty Signed-off-by: Michael Froh <froh@amazon.com> * Update server/src/main/java/org/opensearch/search/pipeline/SearchPipelineService.java Co-authored-by: Navneet Verma <vermanavneet003@gmail.com> Signed-off-by: Michael Froh <froh@amazon.com> * Incorporate feedback from @noCharger Signed-off-by: Michael Froh <froh@amazon.com> * Incorporate feedback from @reta - Renamed various classes from "SearchPipelinesSomething" to "SearchPipelineSomething" to be consistent. - Refactored NodeInfo construction in NodeService to avoid ternary operator and improved readability. Signed-off-by: Michael Froh <froh@amazon.com> * Gate search pipelines behind a feature flag Also renamed SearchPipelinesRequestConverters. Signed-off-by: Michael Froh <froh@amazon.com> * More feature flag fixes for search pipeline testing - Don't use system properties for SearchPipelineServiceTests. - Enable feature flag for multinode smoke tests. Signed-off-by: Michael Froh <froh@amazon.com> * Move feature flag into constructor parameter Thanks for the suggestion, @reta! Signed-off-by: Michael Froh <froh@amazon.com> * Move REST handlers behind feature flag Signed-off-by: Michael Froh <froh@amazon.com> --------- Signed-off-by: Michael Froh <froh@amazon.com> Co-authored-by: Navneet Verma <vermanavneet003@gmail.com> (cherry picked from commit ee990bd)
Gradle Check (Jenkins) Run Completed with:
|
Oops -- looks like the merge misunderstood how to handle these version checks:
I'll go back and pull the one |
1. Can't reference version 3.0.0. 2. Bad merges of adjacent version checks. 3. Use of Apache HTTP client 4 (vs 5). 4. Use of old cluster manager naming in REST params. 5. CollectionUtils didn't have isEmpty for collections. Signed-off-by: Michael Froh <froh@amazon.com>
c1e8cea
to
744d5f9
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Michael Froh <froh@amazon.com>
5ba734e
to
4126cfc
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## 2.x #7075 +/- ##
============================================
+ Coverage 70.35% 70.81% +0.46%
- Complexity 59492 60043 +551
============================================
Files 4822 4849 +27
Lines 285965 286731 +766
Branches 41562 41649 +87
============================================
+ Hits 201178 203042 +1864
+ Misses 67995 67079 -916
+ Partials 16792 16610 -182
... and 478 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@@ -102,6 +103,9 @@ public NodeInfo(StreamInput in) throws IOException { | |||
if (in.getVersion().onOrAfter(LegacyESVersion.V_7_10_0)) { | |||
addInfoIfNonNull(AggregationInfo.class, in.readOptionalWriteable(AggregationInfo::new)); | |||
} | |||
if (in.getVersion().onOrAfter(Version.V_2_7_0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msfroh we would need to bring these changes to main
as well, could you please prepare the pull request so we will merge it right after this one? thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks! Here we go: #7135
I'm canceling this PR for now, just to make sure it doesn't go out in its current state for 2.7. I'll see about reopening after 2.7 is released. |
Talked it over with a colleague, who mentioned that the feature flag is enough that it should be safe to ship this code in 2.7. |
Gradle Check (Jenkins) Run Completed with:
|
@andrross just to confirm, good to go for 2.7.0? thank you |
Late here. Im the mystery colleague. As long as we dont break semver and the feature is gated we should be good. |
This commit includes the basic features of search pipelines (see opensearch-project/search-processor#80).
Search pipelines are modeled after ingest pipelines and provide a simple, clean API for components to modify search requests and responses.
With this commit we can:
Later work will include:
Signed-off-by: Michael Froh froh@amazon.com
Bonus: Added an integration test for filter_query request processor.
Signed-off-by: Michael Froh froh@amazon.com
Also added more useful messages to unit tests to explicitly explain what hoops need to be jumped through in order to add a new serializable exception.
Signed-off-by: Michael Froh froh@amazon.com
I had copied some dependencies from ingest-common, but they are not used by search-pipeline-common (yet).
Signed-off-by: Michael Froh froh@amazon.com
Also, add tests to confirm that a pipeline with no processors works fine (as a no-op).
Signed-off-by: Michael Froh froh@amazon.com
Queries are serialized as NamedWritables, so we need to use a NamedWritableRegistry to deserialize.
Signed-off-by: Michael Froh froh@amazon.com
Signed-off-by: Michael Froh froh@amazon.com
Co-authored-by: Navneet Verma vermanavneet003@gmail.com
Signed-off-by: Michael Froh froh@amazon.com
Signed-off-by: Michael Froh froh@amazon.com
Signed-off-by: Michael Froh froh@amazon.com
Also renamed SearchPipelinesRequestConverters.
Signed-off-by: Michael Froh froh@amazon.com
Signed-off-by: Michael Froh froh@amazon.com
Thanks for the suggestion, @reta!
Signed-off-by: Michael Froh froh@amazon.com
Signed-off-by: Michael Froh froh@amazon.com
Signed-off-by: Michael Froh froh@amazon.com
Co-authored-by: Navneet Verma vermanavneet003@gmail.com
(cherry picked from commit ee990bd)