Cleanup RuntimeException and fetchRemoteStorage logic in ClientUtils #295
Conversation
Codecov Report
@@ Coverage Diff @@
## master #295 +/- ##
============================================
+ Coverage 60.10% 61.31% +1.20%
+ Complexity 1413 1291 -122
============================================
Files 175 162 -13
Lines 9082 7830 -1252
Branches 872 754 -118
============================================
- Hits 5459 4801 -658
+ Misses 3331 2767 -564
+ Partials 292 262 -30 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -30,15 +30,15 @@ public class ClientUtils { | |||
| // taskAttemptId is rest of 20 bit, max value is 2^20 - 1 | |||
| public static Long getBlockId(long partitionId, long taskAttemptId, long atomicInt) { | |||
| if (atomicInt < 0 || atomicInt > Constants.MAX_SEQUENCE_NO) { | |||
| throw new RuntimeException("Can't support sequence[" + atomicInt | |||
| throw new IllegalArgumentException("Can't support sequence[" + atomicInt | |||
There was a problem hiding this comment.
RssException may be better. Because we will collect the RssException to judge the failure apps caused by RSS.
There was a problem hiding this comment.
The root cause of this problem is illegal argument, perhaps new RssException(msg, new IllegalArgumentException())?
RssException may be better. Because we will collect the RssException to judge the failure apps caused by RSS.
For this use case, I think warpping exception outside may be better, for example:
try {
run_rss();
} catch (e) {
throw new RssException(msg, e);
}There was a problem hiding this comment.
The root cause of this problem is illegal argument, perhaps
new RssException(msg, new IllegalArgumentException())?RssException may be better. Because we will collect the RssException to judge the failure apps caused by RSS.
For this use case, I think warpping exception outside may be better, for example:
try { run_rss(); } catch (e) { throw new RssException(msg, e); }
Make Sense.
|
|
|
Thanks @jerqi for the review. |
What changes were proposed in this pull request?
RuntimeExceptionwith proper specific subclass ofRuntimeException.ClientUtils#fetchRemoteStorage().Why are the changes needed?
RuntimeExceptionis not a good practice because it's difficult to catch (without catching other RE).Does this PR introduce any user-facing change?
No.
How was this patch tested?
ClientUtilsTest is modified to assert more specific exceptions.