Add KnnCircuitBreakerException and modify exception message #1688
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Ryan Bogan <[email protected]>
Signed-off-by: Ryan Bogan <[email protected]>
shatejas
reviewed
May 3, 2024
| package org.opensearch.knn.index; | ||
|
|
||
| public class KnnCircuitBreakerException extends RuntimeException { | ||
| public KnnCircuitBreakerException() { |
There was a problem hiding this comment.
Might be worth removing the default constructor so the client of this class is forced to add a message
There was a problem hiding this comment.
@shatejas What do you think about adding a default message in the constructor instead of removing it entirely?
There was a problem hiding this comment.
As long as clients have enough information to understand what went wrong the default message works.
The intent is not to throw without a proper message considering its a custom exception and might be hard for someone to figure out what went wrong
There was a problem hiding this comment.
After trying to implement the default, it doesn't work because it's extending RuntimeException which needs the super to be called first. I'll just remove the default constructor per your original recommendation.
ryanbogan
requested review from
heemin32,
navneet1v,
VijayanB,
vamshin,
jmazanec15,
naveentatikonda,
junqiu-lei,
martin-gaievski and
luyuncheng
as code owners
|
Rolling upgrade failures are happening on other PR's too and are not related to this change. Created an issue here: #1691 |
Signed-off-by: Ryan Bogan <[email protected]>
shatejas
approved these changes
May 7, 2024
src/main/java/org/opensearch/knn/index/KnnCircuitBreakerException.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Ryan Bogan <[email protected]>
Signed-off-by: Ryan Bogan <[email protected]>
martin-gaievski
approved these changes
May 9, 2024
jmazanec15
approved these changes
May 13, 2024
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
May 13, 2024
* Add KnnCircuitBreakerException and modify exception message Signed-off-by: Ryan Bogan <[email protected]> * Add changelog entry and remove star import Signed-off-by: Ryan Bogan <[email protected]> * Remove default exception constructor Signed-off-by: Ryan Bogan <[email protected]> * Add class description and change parameter Signed-off-by: Ryan Bogan <[email protected]> * Fix javadocs Signed-off-by: Ryan Bogan <[email protected]> --------- Signed-off-by: Ryan Bogan <[email protected]> (cherry picked from commit c315862)
ryanbogan
added a commit
that referenced
this pull request
May 13, 2024
…1697) * Add KnnCircuitBreakerException and modify exception message Signed-off-by: Ryan Bogan <[email protected]> * Add changelog entry and remove star import Signed-off-by: Ryan Bogan <[email protected]> * Remove default exception constructor Signed-off-by: Ryan Bogan <[email protected]> * Add class description and change parameter Signed-off-by: Ryan Bogan <[email protected]> * Fix javadocs Signed-off-by: Ryan Bogan <[email protected]> --------- Signed-off-by: Ryan Bogan <[email protected]> (cherry picked from commit c315862) Co-authored-by: Ryan Bogan <[email protected]>
luyuncheng
pushed a commit
to luyuncheng/k-NN-1
that referenced
this pull request
May 22, 2024
…ch-project#1688) * Add KnnCircuitBreakerException and modify exception message Signed-off-by: Ryan Bogan <[email protected]> * Add changelog entry and remove star import Signed-off-by: Ryan Bogan <[email protected]> * Remove default exception constructor Signed-off-by: Ryan Bogan <[email protected]> * Add class description and change parameter Signed-off-by: Ryan Bogan <[email protected]> * Fix javadocs Signed-off-by: Ryan Bogan <[email protected]> --------- Signed-off-by: Ryan Bogan <[email protected]>
luyuncheng
pushed a commit
to luyuncheng/k-NN-1
that referenced
this pull request
May 22, 2024
…ch-project#1688) * Add KnnCircuitBreakerException and modify exception message Signed-off-by: Ryan Bogan <[email protected]> * Add changelog entry and remove star import Signed-off-by: Ryan Bogan <[email protected]> * Remove default exception constructor Signed-off-by: Ryan Bogan <[email protected]> * Add class description and change parameter Signed-off-by: Ryan Bogan <[email protected]> * Fix javadocs Signed-off-by: Ryan Bogan <[email protected]> --------- Signed-off-by: Ryan Bogan <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Currently, when the circuit breaker is triggered and the user attempts to bulk index documents, an illegal state exception is thrown with a message that is not entirely accurate. This PR introduces a new type of exception,
KnnCircuitBreakerException, to be thrown for any action that fails because the circuit breaker is triggered. In addition, it changes the exception message described above to clarify where exactly the exception is being thrown in the indexing process and provide more information about the circuit breaker.Issues Resolved
#1424
Check List
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.