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

Adding the SearchPhaseResultsProcessor interface in Search Pipeline #7283

Merged
merged 13 commits into from
Jun 29, 2023

Conversation

navneet1v
Copy link
Contributor

@navneet1v navneet1v commented Apr 24, 2023

Description

Add new search pipeline processor type, SearchPhaseResultsProcessor, that can modify the result of one search phase before starting the next phase.

Along with this, added the code to resolve the Search pipeline once and added new SearchRequest type PipelinedRequest.

Issues Resolved

RFC: opensearch-project/neural-search#152

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.

@navneet1v navneet1v requested a review from msfroh April 24, 2023 00:51
@navneet1v navneet1v force-pushed the searchPhaseInjectorProcessor branch from b70ef2e to 7945f01 Compare April 24, 2023 00:53
@navneet1v navneet1v changed the title []Initial code for adding the SearchPhaseInjectorProcessor interface in Search Pipeline Initial code for adding the SearchPhaseInjectorProcessor interface in Search Pipeline Apr 24, 2023
@navneet1v navneet1v added backport 2.x Backport to 2.x branch Search Search query, autocomplete ...etc labels Apr 24, 2023
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@navneet1v navneet1v force-pushed the searchPhaseInjectorProcessor branch from 7945f01 to 81f1a4a Compare April 24, 2023 18:51
@navneet1v navneet1v requested a review from reta June 28, 2023 19:58
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@reta
Copy link
Collaborator

reta commented Jun 28, 2023

@navneet1v needs rebase please

@andrross
Copy link
Member

@navneet1v I just merged the latest from main, which should fix the tests now.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      2 org.opensearch.remotestore.RemoteStoreRefreshListenerIT.testRemoteRefreshRetryOnFailure
      1 org.opensearch.snapshots.RestoreSnapshotIT.testRestoreInSameRemoteStoreEnabledIndex
      1 org.opensearch.search.pit.DeletePitMultiNodeIT.testDeleteWhileSearch
      1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testNodeDropWithOngoingReplication
      1 org.opensearch.action.admin.cluster.node.tasks.ResourceAwareTasksTests.testBasicTaskResourceTracking

@msfroh
Copy link
Collaborator

msfroh commented Jun 28, 2023

I'm working on resolving the (many) conflicts that arose from #8053 getting merged.

I'll post the new commits shortly.

@navneet1v
Copy link
Contributor Author

I'm working on resolving the (many) conflicts that arose from #8053 getting merged.

I'll post the new commits shortly.

Thanks @msfroh for picking it up. You can only resolve these conflicts. :)

@msfroh msfroh force-pushed the searchPhaseInjectorProcessor branch from 81baeda to 096417c Compare June 28, 2023 23:04
@msfroh
Copy link
Collaborator

msfroh commented Jun 28, 2023

Ugh -- I think I clobbered @navneet1v's recent changes by force-pushing after rebase.

I'm going to see if I can recover them (or reapply them manually).

@navneet1v navneet1v force-pushed the searchPhaseInjectorProcessor branch from 096417c to 81baeda Compare June 28, 2023 23:29
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      2 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testDropPrimaryDuringReplication
      1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testNodeDropWithOngoingReplication
      1 org.opensearch.remotestore.RemoteStoreRefreshListenerIT.testRemoteRefreshRetryOnFailure
      1 org.opensearch.indices.replication.SegmentReplicationIT.testCancellation
      1 org.opensearch.cluster.allocation.AwarenessAllocationIT.testThreeZoneOneReplicaWithForceZoneValueAndLoadAwareness

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.snapshots.RestoreSnapshotIT.testRestoreInSameRemoteStoreEnabledIndex
      1 org.opensearch.search.SearchWeightedRoutingIT.testSearchAggregationWithNetworkDisruption_FailOpenEnabled
      1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testNodeDropWithOngoingReplication

Signed-off-by: Michael Froh <froh@amazon.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testNodeDropWithOngoingReplication
      1 org.opensearch.indices.replication.SegmentReplicationRelocationIT.testPrimaryRelocation

@reta reta added the v2.9.0 'Issues and PRs related to version v2.9.0' label Jun 29, 2023
@reta reta merged commit b33979a into opensearch-project:main Jun 29, 2023
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-7283-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b33979a4d95f728ee3080eb6983d93e024fb7ba2
# Push it to GitHub
git push --set-upstream origin backport/backport-7283-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-7283-to-2.x.

@reta
Copy link
Collaborator

reta commented Jun 29, 2023

@navneet1v needs manual backport to 2.x sadly, could you please take care of it? thank you.

@navneet1v
Copy link
Contributor Author

@navneet1v needs manual backport to 2.x sadly, could you please take care of it? thank you.

Yes I will take care of that. Thanks everyone. This PR was pending for long time and took more effort than I expected.

navneet1v added a commit to navneet1v/OpenSearch that referenced this pull request Jul 7, 2023
…pensearch-project#7283)

* Initial code for adding the SearchPhaseInjectorProcessor interface in Search Pipeline

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Pass PipelinedRequest to SearchAsyncActions

We should resolve a search pipeline once at the start of a search
request and then propagate that pipeline through the async actions.

When completing a search phase, we will then use that pipeline to inject
behavior (if applicable).

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

* Renamed SearchPhaseInjectorProcessor to SearchPhaseResultsProcessor and fixed the comments

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Make PipelinedSearchRequest extend SearchRequest

Rather than wrapping a SearchRequest in a PipelinedSearchRequest,
changes are less intrusive if we say that a PipelinedSearchRequest
"is a" SearchRequest.

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

* Revert code change from merge conflict

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

* Updated the changelog with more appropiate wording for the change.

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Fixed Typos in the code

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Fixing comments relating to return of SearchPhaseResults from processor

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Moved SearchPhaseName enum in separate class and fixed comments.

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Resolve remaining merge conflict

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

---------

Signed-off-by: Navneet Verma <navneev@amazon.com>
Signed-off-by: Michael Froh <froh@amazon.com>
Co-authored-by: Michael Froh <froh@amazon.com>
Co-authored-by: Andrew Ross <andrross@amazon.com>
tlfeng pushed a commit that referenced this pull request Jul 7, 2023
…7283) (#8512)

Add new search pipeline processor type, SearchPhaseResultsProcessor, that can modify the result of one search phase before starting the next phase.

Along with this, added the code to resolve the Search pipeline once and added new SearchRequest type PipelinedRequest.

Backport of PR: #7283

---------

Signed-off-by: Navneet Verma <navneev@amazon.com>
Co-authored-by: Michael Froh <froh@amazon.com>
Co-authored-by: Andrew Ross <andrross@amazon.com>
baba-devv pushed a commit to baba-devv/OpenSearch that referenced this pull request Jul 29, 2023
…pensearch-project#7283)

* Initial code for adding the SearchPhaseInjectorProcessor interface in Search Pipeline

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Pass PipelinedRequest to SearchAsyncActions

We should resolve a search pipeline once at the start of a search
request and then propagate that pipeline through the async actions.

When completing a search phase, we will then use that pipeline to inject
behavior (if applicable).

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

* Renamed SearchPhaseInjectorProcessor to SearchPhaseResultsProcessor and fixed the comments

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Make PipelinedSearchRequest extend SearchRequest

Rather than wrapping a SearchRequest in a PipelinedSearchRequest,
changes are less intrusive if we say that a PipelinedSearchRequest
"is a" SearchRequest.

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

* Revert code change from merge conflict

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

* Updated the changelog with more appropiate wording for the change.

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Fixed Typos in the code

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Fixing comments relating to return of SearchPhaseResults from processor

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Moved SearchPhaseName enum in separate class and fixed comments.

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Resolve remaining merge conflict

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

---------

Signed-off-by: Navneet Verma <navneev@amazon.com>
Signed-off-by: Michael Froh <froh@amazon.com>
Co-authored-by: Michael Froh <froh@amazon.com>
Co-authored-by: Andrew Ross <andrross@amazon.com>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…pensearch-project#7283)

* Initial code for adding the SearchPhaseInjectorProcessor interface in Search Pipeline

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Pass PipelinedRequest to SearchAsyncActions

We should resolve a search pipeline once at the start of a search
request and then propagate that pipeline through the async actions.

When completing a search phase, we will then use that pipeline to inject
behavior (if applicable).

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

* Renamed SearchPhaseInjectorProcessor to SearchPhaseResultsProcessor and fixed the comments

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Make PipelinedSearchRequest extend SearchRequest

Rather than wrapping a SearchRequest in a PipelinedSearchRequest,
changes are less intrusive if we say that a PipelinedSearchRequest
"is a" SearchRequest.

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

* Revert code change from merge conflict

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

* Updated the changelog with more appropiate wording for the change.

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Fixed Typos in the code

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Fixing comments relating to return of SearchPhaseResults from processor

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Moved SearchPhaseName enum in separate class and fixed comments.

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Resolve remaining merge conflict

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

---------

Signed-off-by: Navneet Verma <navneev@amazon.com>
Signed-off-by: Michael Froh <froh@amazon.com>
Co-authored-by: Michael Froh <froh@amazon.com>
Co-authored-by: Andrew Ross <andrross@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 Search Search query, autocomplete ...etc v2.9.0 'Issues and PRs related to version v2.9.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants