11246 :: Unexpected error when server expands a compressed message to learn it is too large #12360
Conversation
… learn it is too large
… learn it is too large
vimanikag
changed the title
… learn it is too large
| if (t instanceof StatusRuntimeException) { | ||
| if (((StatusRuntimeException) t).getStatus().getCode() == Status.Code.RESOURCE_EXHAUSTED) { | ||
| statusToPropagate = ((StatusRuntimeException) t).getStatus().withCause(t); | ||
| } | ||
| } |
There was a problem hiding this comment.
This can be refactored to use a single if. But, personally I would extract out the explicit conversion of t into a variable.
There was a problem hiding this comment.
Thanks for the review, @AgraVator . I've addressed the comments.
| ServerStreamListener mockListener = mock(ServerStreamListener.class); | ||
| listener.setListener(mockListener); | ||
|
|
||
| RuntimeException expectedT = new RuntimeException(); |
There was a problem hiding this comment.
@AgraVator , The new test cases are inspired by the existing junit's like onReady_runtimeExceptionCancelsCall()/halfClosed_runtimeExceptionCancelsCall , following the same naming format. Please advise if you would prefer to change the variable to expected if We suspect it's a typo or if the current naming was intentional.
| if (((StatusRuntimeException) t).getStatus().getCode() == Status.Code.RESOURCE_EXHAUSTED) { | ||
| statusToPropagate = ((StatusRuntimeException) t).getStatus().withCause(t); |
There was a problem hiding this comment.
Are there no other cases where we can get "RESOURCE_EXHAUSTED" ? As, this is not tied specifically to marshaling the intended behavior can change if and when new "RESOURCE_EXHAUSTED" are added.
There was a problem hiding this comment.
@AgraVator , I've addressed the review comments and changed the logic to handle both StatusRuntimeException and StatusException , I also maintained the existing exception propagation as same for other status code except RESOURCE_EXHAUSTED, Please review and share your feedback.
Fixes : #11246