-
Notifications
You must be signed in to change notification settings - Fork 151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#2269] refactor: Fix duplicated blockIds issue caused by duplicated reportShuffleResult #2270
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2270 +/- ##
============================================
- Coverage 51.78% 51.76% -0.03%
+ Complexity 2966 2965 -1
============================================
Files 479 479
Lines 22566 22569 +3
Branches 2068 2069 +1
============================================
- Hits 11686 11683 -3
- Misses 10140 10149 +9
+ Partials 740 737 -3 ☔ View full report in Codecov by Sentry. |
@@ -797,6 +798,11 @@ private ReportShuffleResultResponse doReportShuffleResult(ReportShuffleResultReq | |||
try { | |||
ReportShuffleResultResponse response = getBlockingStub().reportShuffleResult(rpcRequest); | |||
return response; | |||
} catch (StatusRuntimeException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you reuse RetryUtils
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@jerqi Thank you very much to inspire me to improve the code to be better. PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@jerqi Sorry, there is a spotbug issue and I just fixed it. PTAL |
What changes were proposed in this pull request?
Client stop retrying and throw RssException for reportShuffleResult operation while interrupted by caller.
Why are the changes needed?
Fix: #2269
Does this PR introduce any user-facing change?
No.
How was this patch tested?
No need