Painless compilation threshold breach with correct error status #1066
Conversation
|
✅ Gradle Wrapper Validation success f8ceccf |
|
✅ DCO Check Passed f8ceccf |
|
✅ Gradle Precommit success f8ceccf |
| } else if (cause instanceof CircuitBreakingException) { | ||
| throw new OpenSearchRejectedExecutionException( | ||
| "Failed to compile " + type + " script [" + id + "] using lang [" + lang + "]", cause); |
There was a problem hiding this comment.
CircuitBreakingException already returns 429?
There was a problem hiding this comment.
CircuitBreakingException was wrapped in GeneralScriptException which return 500 error. You can have a look at reproduced error.
There was a problem hiding this comment.
Ideally the status is determined by the underlying cause https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/OpenSearchException.java#L253-L260. . In this case since CircuitBreakingException is the underlying cause, the error code should have been 429
Can you please confirm if the below is not being invoked.
public static RestStatus status(Throwable t) {
if (t != null) {
if (t instanceof OpenSearchException) {
return ((OpenSearchException) t).status();
} else if (t instanceof IllegalArgumentException) {
return RestStatus.BAD_REQUEST;
} else if (t instanceof JsonParseException) {
return RestStatus.BAD_REQUEST;
} else if (t instanceof OpenSearchRejectedExecutionException) {
return RestStatus.TOO_MANY_REQUESTS;
}
}
return RestStatus.INTERNAL_SERVER_ERROR;
}
Also please update the steps to reproduce
|
✅ Gradle Wrapper Validation success f1973da |
|
✅ DCO Check Passed f1973da |
|
✅ Gradle Precommit success f1973da |
|
Pinging @dblock @adnapibar for help with approval and merge |
|
start gradle check |
|
Whatever the external behavior change here, it's not captured. I think this needs a test. |
|
Updated the linked issue with better reproduction steps. |
|
Earlier when @dblock do you want me to create a unit test to check this behaviour change? |
Yes, a test that shows that the response is now a 429, instead of a 500. |
|
✅ Gradle Wrapper Validation success bfce557 |
|
✅ DCO Check Passed bfce557 |
|
@Bukhtawar @dblock created a test to check the status. Please review the changes. Any feedback is welcome 😄 . |
The unit test is great, it happens at the lowest compilation level, keep it! Because the original issue and the repro is at REST level, try also adding a REST test? One that makes an HTTP request, and gets back a 429 whereas before this fix it would cause a 500? Check the pre-commit failure, too. |
I manually tested code by running the commands. Is the there any example in the codebase to run automated REST test? |
|
✅ DCO Check Passed 9376e28 |
|
✅ Gradle Wrapper Validation success 9376e28 |
|
✅ Gradle Precommit success 9376e28 |
shiv0408
changed the title
|
start gradle check |
|
@dblock Can you please go through the integTest I created? |
|
✅ DCO Check Passed e7732078552e92d984c63c8d39e8a77b87af1788 |
|
✅ Gradle Wrapper Validation success e7732078552e92d984c63c8d39e8a77b87af1788 |
|
❌ Gradle Precommit failure e7732078552e92d984c63c8d39e8a77b87af1788 |
|
✅ Gradle Wrapper Validation success fe8414e8778f8c1b8fb98ad0710af6a2fd9f563a |
|
✅ DCO Check Passed fe8414e8778f8c1b8fb98ad0710af6a2fd9f563a |
|
✅ Gradle Precommit success fe8414e8778f8c1b8fb98ad0710af6a2fd9f563a |
|
start gradle check |
|
❌ Gradle Check failure fe8414e8778f8c1b8fb98ad0710af6a2fd9f563a |
|
start gradle check |
|
❌ Gradle Check failure fe8414e8778f8c1b8fb98ad0710af6a2fd9f563a |
|
✅ Gradle Wrapper Validation success eae8a31348323f0197fd536ec293ef7139a98470 |
|
❌ DCO Check Failed eae8a31348323f0197fd536ec293ef7139a98470 |
|
✅ Gradle Precommit success eae8a31348323f0197fd536ec293ef7139a98470 |
…xception Signed-off-by: Shivansh Arora <hishiv@amazon.com>
shiv0408
force-pushed
the
painless-threshold-rejection
branch
from
eae8a31
to
aef5077
Compare
|
start gradle check |
|
✅ Gradle Wrapper Validation success aef5077 |
|
✅ DCO Check Passed aef5077 |
|
✅ Gradle Precommit success aef5077 |
Description
When a circuit breaker occurs due to compilation threshold breach, then OpenSearchRejectedExecutionException is called.
Issues Resolved
Resolves #1064
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.