Add Retryable/Non-Retryable Exception + API Calls Metrics for O365#6238
Conversation
Signed-off-by: Brett Zeligson <brettzel@amazon.com>
zeligsonbrett
requested review from
KarstenSchnitter,
chenqi0805,
dinujoh,
dlvenable,
engechas,
graytaylor0,
kkondaka,
oeyh,
san81,
sb2k16 and
srikanthjg
as code owners
| @@ -130,6 +136,7 @@ public void executePartition(final DimensionalTimeSliceWorkerProgressState state | |||
| if (e.isRetryable()) { | |||
There was a problem hiding this comment.
missing to increase the retryable error counter here? Simply calling handleExceptionMetrics method from here without this if else checking would be better.
There was a problem hiding this comment.
Since this is throwing a RuntimeException, it will get caught in the outer catch which contains the handleExceptionMetrics(e) call.
There was a problem hiding this comment.
No. It doesn't work that way, since you are throwing a RuntimeException. It won't be an instance of Office365 exception
There was a problem hiding this comment.
Yes, but from my understanding when the RuntimeException is thrown from within this if statement, it would get caught in the outer catch which contains the handleExceptionMetrics() call which would increment the retryableErrorsCounter within handleExceptionMetrics() because the caught error would be a RuntimeException (not an Office365Exception anymore) as you said.
|
|
||
| private void handleExceptionMetrics(Exception e) { | ||
| if (e instanceof Office365Exception) { | ||
| if (((Office365Exception) e).isRetryable()) { |
There was a problem hiding this comment.
Is this code reachable? It seems that you catch this above and do not record the metrics.
I made a similar comment elsewhere: #6202 (comment)
Let's just put the .increment() calls in the conditions where you handle the exceptions.
Signed-off-by: Brett Zeligson <brettzel@amazon.com>
Signed-off-by: Brett Zeligson <brettzel@amazon.com>
…search-project#6253) Signed-off-by: Vecheka <vecheka@amazon.com>
dlvenable
left a comment
dlvenable
left a comment
There was a problem hiding this comment.
Thanks @zeligsonbrett . This looks good to me! You will need to rebase from main since there is a conflict.
…search-project#6253) Signed-off-by: Vecheka <vecheka@amazon.com>
Signed-off-by: Brett Zeligson <brettzel@amazon.com>
zeligsonbrett
force-pushed
the
main
branch
2 times, most recently
from
57f5b32 to
6c5ed31
Compare
Signed-off-by: Brett Zeligson <85852739+zeligsonbrett@users.noreply.github.com>
zeligsonbrett
dismissed
san81’s stale review
The merge-base changed after approval.
4 participants
Description
Add Retryable/Non-Retryable Exception + API Calls Metrics for O365
Issues Resolved
This change does not directly resolve any open issues.
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.