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

[Search Pipelines] Split search pipeline processor factories by type #7597

Merged
merged 4 commits into from
May 22, 2023

Conversation

msfroh
Copy link
Collaborator

@msfroh msfroh commented May 17, 2023

Description

In the initial search pipelines commit, I threw request and response processor factories into one combined map. I think that was a mistake.

We should embrace type-safety by making sure that the kind of processor is clear from end to end. As we add more processor types (e.g. search phase processor), throwing them all in one big map would get messier.

As a bonus, we'll be able to reuse processor names across different types of processor.

Related Issues

Resolves #7576

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:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@msfroh msfroh force-pushed the search_pipelines_namespaces branch from 139e591 to 76e3245 Compare May 17, 2023 17:40
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov
Copy link

codecov bot commented May 17, 2023

Codecov Report

Merging #7597 (63f0fa7) into main (df7cf27) will increase coverage by 0.06%.
The diff coverage is 83.52%.

❗ Current head 63f0fa7 differs from pull request most recent head faabc42. Consider uploading reports for the commit faabc42 to get more accurate results

@@             Coverage Diff              @@
##               main    #7597      +/-   ##
============================================
+ Coverage     70.80%   70.87%   +0.06%     
- Complexity    56085    56118      +33     
============================================
  Files          4676     4676              
  Lines        265927   265895      -32     
  Branches      39053    39051       -2     
============================================
+ Hits         188292   188446     +154     
+ Misses        61678    61474     -204     
- Partials      15957    15975      +18     
Impacted Files Coverage Δ
...h/pipeline/common/FilterQueryRequestProcessor.java 85.29% <ø> (ø)
.../pipeline/common/RenameFieldResponseProcessor.java 94.59% <ø> (ø)
...eline/common/SearchPipelineCommonModulePlugin.java 0.00% <0.00%> (ø)
...a/org/opensearch/plugins/SearchPipelinePlugin.java 0.00% <0.00%> (ø)
...java/org/opensearch/search/pipeline/Processor.java 100.00% <ø> (ø)
...opensearch/search/pipeline/SearchPipelineInfo.java 72.41% <84.44%> (+36.69%) ⬆️
...nsearch/search/pipeline/SearchPipelineService.java 82.77% <88.23%> (-0.04%) ⬇️
.../java/org/opensearch/search/pipeline/Pipeline.java 86.20% <100.00%> (+1.59%) ⬆️

... and 463 files with indirect coverage changes

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.remotestore.SegmentReplicationRemoteStoreIT.testReplicaHasDiffFilesThanPrimary

@msfroh msfroh changed the title Split search pipeline processor factories by type [Search Pipelines] Split search pipeline processor factories by type May 18, 2023
@msfroh msfroh force-pushed the search_pipelines_namespaces branch from 4a5c9bc to 63f0fa7 Compare May 19, 2023 18:06
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@msfroh
Copy link
Collaborator Author

msfroh commented May 19, 2023

  • a flaky test

Flaky test: #7592

@msfroh
Copy link
Collaborator Author

msfroh commented May 19, 2023

Looking for some feedback on this change. @reta @andrross @navneet1v ?

I would like to get it in for 2.8, since it will be much harder to change if/when we release the search pipelines feature more broadly.

@macohen
Copy link
Contributor

macohen commented May 21, 2023

can we have a reviewer in addition to @reta and @andrross take a look at this please? @VachaShah @kartg @saratvemulapalli @tlfeng, perhaps?

thanks!

Copy link
Collaborator

@nknize nknize left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty straightforward, overall LGTM. Just one Q on thread safety.

@nknize
Copy link
Collaborator

nknize commented May 22, 2023

Le sigh.. another unrelated failure. #7592

These "flaky" tests need some attention. They're starting to get unmanageable and gridlock our PRs.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@nknize
Copy link
Collaborator

nknize commented May 22, 2023

I'm happy to approve w/ the minor changes above. Don't worry about unrelated flaky tests here. We'll fix those separately.

msfroh added 4 commits May 22, 2023 19:15
In the initial search pipelines commit, I threw request and response
processor factories into one combined map. I think that was a mistake.

We should embrace type-safety by making sure that the kind of processor
is clear from end to end. As we add more processor types (e.g. search
phase processor), throwing them all in one big map would get messier.

As a bonus, we'll be able to reuse processor names across different
types of processor.

Closes opensearch-project#7576

Signed-off-by: Michael Froh <froh@amazon.com>
Signed-off-by: Michael Froh <froh@amazon.com>
Signed-off-by: Michael Froh <froh@amazon.com>
Thanks @nknize!

Signed-off-by: Michael Froh <froh@amazon.com>
@msfroh msfroh force-pushed the search_pipelines_namespaces branch from 63f0fa7 to faabc42 Compare May 22, 2023 19:15
Copy link
Collaborator

@nknize nknize left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. 🤞🏻 we don't hit flaky test hell...

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@nknize nknize merged commit eb51f2b into opensearch-project:main May 22, 2023
@msfroh msfroh added the backport 2.x Backport to 2.x branch label May 22, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 22, 2023
…7597)

In the initial search pipelines commit, I threw request and response
processor factories into one combined map. I think that was a mistake.

We should embrace type-safety by making sure that the kind of processor
is clear from end to end. As we add more processor types (e.g. search
phase processor), throwing them all in one big map would get messier.

As a bonus, we'll be able to reuse processor names across different
types of processor.

Signed-off-by: Michael Froh <froh@amazon.com>
(cherry picked from commit eb51f2b)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
peterzhuamazon pushed a commit that referenced this pull request May 22, 2023
…tories by type (#7669)

* [Search Pipelines] Split search pipeline processor factories by type (#7597)

In the initial search pipelines commit, I threw request and response
processor factories into one combined map. I think that was a mistake.

We should embrace type-safety by making sure that the kind of processor
is clear from end to end. As we add more processor types (e.g. search
phase processor), throwing them all in one big map would get messier.

As a bonus, we'll be able to reuse processor names across different
types of processor.

Signed-off-by: Michael Froh <froh@amazon.com>
(cherry picked from commit eb51f2b)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Update SearchPipelineInfo serialization check to < 2.8

We needed to check for onOrBefore 2.8 in order to pass back-compat
tests on main. Now that we're backporting to 2.8, we need to update the
check (on 2.x and on main).

Signed-off-by: Michael Froh <froh@amazon.com>

---------

Signed-off-by: Michael Froh <froh@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
bharath-techie pushed a commit to bharath-techie/OpenSearch that referenced this pull request May 23, 2023
…pensearch-project#7597)

In the initial search pipelines commit, I threw request and response
processor factories into one combined map. I think that was a mistake.

We should embrace type-safety by making sure that the kind of processor
is clear from end to end. As we add more processor types (e.g. search
phase processor), throwing them all in one big map would get messier.

As a bonus, we'll be able to reuse processor names across different
types of processor.

Signed-off-by: Michael Froh <froh@amazon.com>
Signed-off-by: Bharathwaj G <bharath78910@gmail.com>
suranjay pushed a commit to suranjay/OpenSearch that referenced this pull request May 29, 2023
…pensearch-project#7597)

In the initial search pipelines commit, I threw request and response
processor factories into one combined map. I think that was a mistake.

We should embrace type-safety by making sure that the kind of processor
is clear from end to end. As we add more processor types (e.g. search
phase processor), throwing them all in one big map would get messier.

As a bonus, we'll be able to reuse processor names across different
types of processor.

Signed-off-by: Michael Froh <froh@amazon.com>
stephen-crawford pushed a commit to stephen-crawford/OpenSearch that referenced this pull request May 31, 2023
…pensearch-project#7597)

In the initial search pipelines commit, I threw request and response
processor factories into one combined map. I think that was a mistake.

We should embrace type-safety by making sure that the kind of processor
is clear from end to end. As we add more processor types (e.g. search
phase processor), throwing them all in one big map would get messier.

As a bonus, we'll be able to reuse processor names across different
types of processor.

Signed-off-by: Michael Froh <froh@amazon.com>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…pensearch-project#7597)

In the initial search pipelines commit, I threw request and response
processor factories into one combined map. I think that was a mistake.

We should embrace type-safety by making sure that the kind of processor
is clear from end to end. As we add more processor types (e.g. search
phase processor), throwing them all in one big map would get messier.

As a bonus, we'll be able to reuse processor names across different
types of processor.

Signed-off-by: Michael Froh <froh@amazon.com>
Signed-off-by: Shivansh Arora <hishiv@amazon.com>
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
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Search Pipelines] Split processor namespace by type (request vs response vs phase)
3 participants