[Feature/multi_tenancy] Restore original exception handling expectations #2507
Conversation
dbwiddis
requested review from
b4sjoo,
dhrubo-os,
jngz-es,
model-collapse,
rbhavna,
ylwu-amzn,
zane-neo,
Zhangxunmt,
austintlee,
HenryL27 and
sam-herman
as code owners
dbwiddis
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Error
dbwiddis
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Error
dbwiddis
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Failure
dbwiddis
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Error
dbwiddis
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Failure
dbwiddis
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Error
dbwiddis
force-pushed
the
exception-handling
branch
from
6f20123 to
9ddfe16
Compare
dbwiddis
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Failure
dbwiddis
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Error
dbwiddis
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Error
dbwiddis
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Error
dbwiddis
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Error
dbwiddis
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Failure
Signed-off-by: Daniel Widdis <[email protected]>
dbwiddis
force-pushed
the
exception-handling
branch
from
9ddfe16 to
22a0ce5
Compare
dbwiddis
temporarily deployed
to
ml-commons-cicd-env
GitHub Actions
Inactive
dbwiddis
temporarily deployed
to
ml-commons-cicd-env
GitHub Actions
Inactive
dbwiddis
temporarily deployed
to
ml-commons-cicd-env
GitHub Actions
Inactive
dbwiddis
temporarily deployed
to
ml-commons-cicd-env
GitHub Actions
Inactive
dbwiddis
temporarily deployed
to
ml-commons-cicd-env
GitHub Actions
Inactive
dbwiddis
temporarily deployed
to
ml-commons-cicd-env
GitHub Actions
Inactive
dbwiddis
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Failure
dbwiddis
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Failure
dbwiddis
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Error
| @@ -114,7 +124,12 @@ default DeleteDataObjectResponse deleteDataObject(DeleteDataObjectRequest reques | |||
| if (cause instanceof InterruptedException) { | |||
| Thread.currentThread().interrupt(); | |||
| } | |||
| throw new OpenSearchException(cause); | |||
| // Rethrow unchecked Exceptions | |||
| if (cause instanceof RuntimeException) { | |||
There was a problem hiding this comment.
what about other Exceptions like: IllegalArgumentException, IOException, MLException
There was a problem hiding this comment.
IllegalArgumentException and MLException are both subclasses of RuntimeException so will be thrown here as the original exception.
IOException is a checked exception so is wrapped in an unchecked exception before throwing.
See the PR description where I suggested possibly wrapping in an UndeclaredThrowableException instead of OpenSearchException.
Do you have an alternative proposal here? If we don't rethrow checked exceptions we need to add throws clauses to every one of these API methods.
There was a problem hiding this comment.
We have MLException class, may be we can re-use them?
There was a problem hiding this comment.
These clients aren't unique to ML Commons. The exceptions should be generic. This class will not be in ML Commons eventually and shouldn't need to add it as a dependency.
The intent here is to rethrow the original exception whenever possible, and if not, pass a throwable with the original exception as its cause.
| // Rethrow unchecked Exceptions | ||
| if (cause instanceof RuntimeException) { | ||
| throw (RuntimeException) cause; | ||
| } else { | ||
| throw new OpenSearchException(cause); | ||
| } |
There was a problem hiding this comment.
Given that CompletionException extends RunTimeException do we except non-RunTimeException?
There was a problem hiding this comment.
Do we "expect"? No. But I spent quite some time debugging a mysterious NullPointerException which was swallowed without handling like this.
There was a problem hiding this comment.
Also, I've already extracted the cause from the Completion Exception:
Throwable cause = e.getCause();
|
closing this PR in favor of #2520 |
Description
Restores the original exception handling behavior for connector transport actions
Issues Resolved
Resolves TODO from #2459
Open question: should the non-runtime exceptions be wrapped in an
UndeclaredThrowableExceptionrather than anOpenSearchStatusException? I'm thinking probably.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.