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

[Bugfix] Better exception handling in search pipelines #7735

Merged

Conversation

msfroh
Copy link
Collaborator

@msfroh msfroh commented May 24, 2023

Description

Thanks to @noCharger for reporting a failing negative test case.

Since we were rethrowing exceptions when resolving search pipelines and processing search requests, that could end up killing the listener thread.

Also, we want to make sure that any exception thrown from search pipelines are wrapped in SearchPipelineProcessingException.

Related Issues

N/A

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.

Thanks to @noCharger for reporting a failing negative test case.

Since we were rethrowing exceptions when resolving search pipelines and
processing search requests, that could end up killing the listener
thread.

Also, we want to make sure that any exception thrown from search
pipelines are wrapped in SearchPipelineProcessingException.

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

@noCharger noCharger left a comment

Choose a reason for hiding this comment

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

Thanks for the instant bugfix. Let's add changelog and also one unit test case for SearchPipelineService.java

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.search.backpressure.SearchBackpressureIT.testSearchShardTaskCancellationWithHighCpu
      1 org.opensearch.indices.replication.SegmentReplicationIT.testScrollCreatedOnReplica

@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Merging #7735 (681210c) into main (70b109d) will increase coverage by 0.12%.
The diff coverage is 80.00%.

@@             Coverage Diff              @@
##               main    #7735      +/-   ##
============================================
+ Coverage     70.63%   70.76%   +0.12%     
- Complexity    56099    56162      +63     
============================================
  Files          4680     4680              
  Lines        266083   266085       +2     
  Branches      39074    39074              
============================================
+ Hits         187955   188296     +341     
+ Misses        62187    61808     -379     
- Partials      15941    15981      +40     
Impacted Files Coverage Δ
...pensearch/action/search/TransportSearchAction.java 70.99% <0.00%> (+0.43%) ⬆️
...nsearch/search/pipeline/SearchPipelineService.java 83.88% <100.00%> (+1.11%) ⬆️

... and 463 files with indirect coverage changes

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

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

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.indices.replication.SegmentReplicationIT.testScrollCreatedOnReplica

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@msfroh
Copy link
Collaborator Author

msfroh commented May 24, 2023

Failing tests on Gradle check seem to be related to #7679

(Not exactly the same failure -- this one is failing because the index already exists, which I saw on a previous PR.)

@msfroh msfroh added the backport 2.x Backport to 2.x branch label May 25, 2023
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.repositories.s3.S3BlobContainerRetriesTests.testReadRangeBlobWithRetries

@reta reta merged commit cf02b96 into opensearch-project:main May 25, 2023
13 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 25, 2023
* [Bugfix] Better exception handling in search pipelines

Thanks to @noCharger for reporting a failing negative test case.

Since we were rethrowing exceptions when resolving search pipelines and
processing search requests, that could end up killing the listener
thread.

Also, we want to make sure that any exception thrown from search
pipelines are wrapped in SearchPipelineProcessingException.

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

* Add changelog entry and unit tests

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

* Add check on error message for negative test

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

* Fix misleading commment in test

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

---------

Signed-off-by: Michael Froh <froh@amazon.com>
(cherry picked from commit cf02b96)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
reta pushed a commit that referenced this pull request May 25, 2023
* [Bugfix] Better exception handling in search pipelines

Thanks to @noCharger for reporting a failing negative test case.

Since we were rethrowing exceptions when resolving search pipelines and
processing search requests, that could end up killing the listener
thread.

Also, we want to make sure that any exception thrown from search
pipelines are wrapped in SearchPipelineProcessingException.



* Add changelog entry and unit tests



* Add check on error message for negative test



* Fix misleading commment in test



---------


(cherry picked from commit cf02b96)

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>
dblock pushed a commit to dblock/OpenSearch that referenced this pull request May 25, 2023
…oject#7735)

* [Bugfix] Better exception handling in search pipelines

Thanks to @noCharger for reporting a failing negative test case.

Since we were rethrowing exceptions when resolving search pipelines and
processing search requests, that could end up killing the listener
thread.

Also, we want to make sure that any exception thrown from search
pipelines are wrapped in SearchPipelineProcessingException.

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

* Add changelog entry and unit tests

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

* Add check on error message for negative test

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

* Fix misleading commment in test

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

---------

Signed-off-by: Michael Froh <froh@amazon.com>
suranjay pushed a commit to suranjay/OpenSearch that referenced this pull request May 29, 2023
…oject#7735)

* [Bugfix] Better exception handling in search pipelines

Thanks to @noCharger for reporting a failing negative test case.

Since we were rethrowing exceptions when resolving search pipelines and
processing search requests, that could end up killing the listener
thread.

Also, we want to make sure that any exception thrown from search
pipelines are wrapped in SearchPipelineProcessingException.

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

* Add changelog entry and unit tests

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

* Add check on error message for negative test

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

* Fix misleading commment in test

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

---------

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

* [Bugfix] Better exception handling in search pipelines

Thanks to @noCharger for reporting a failing negative test case.

Since we were rethrowing exceptions when resolving search pipelines and
processing search requests, that could end up killing the listener
thread.

Also, we want to make sure that any exception thrown from search
pipelines are wrapped in SearchPipelineProcessingException.

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

* Add changelog entry and unit tests

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

* Add check on error message for negative test

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

* Fix misleading commment in test

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

---------

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

* [Bugfix] Better exception handling in search pipelines

Thanks to @noCharger for reporting a failing negative test case.

Since we were rethrowing exceptions when resolving search pipelines and
processing search requests, that could end up killing the listener
thread.

Also, we want to make sure that any exception thrown from search
pipelines are wrapped in SearchPipelineProcessingException.

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

* Add changelog entry and unit tests

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

* Add check on error message for negative test

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

* Fix misleading commment in test

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

---------

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 skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants