Skip to content

Conversation

@kaijchen
Copy link
Member

@kaijchen kaijchen commented Nov 2, 2022

What changes were proposed in this pull request?

  1. Replace raw RuntimeException with proper specific subclass of RuntimeException.
  2. Cleanup the logic in ClientUtils#fetchRemoteStorage().

Why are the changes needed?

  1. Throwing RuntimeException is not a good practice because it's difficult to catch (without catching other RE).
  2. For clearity and easier to understand.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

ClientUtilsTest is modified to assert more specific exceptions.

@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2022

Codecov Report

Merging #295 (399765c) into master (0935af2) will increase coverage by 1.20%.
The diff coverage is 50.00%.

@@             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     
Impacted Files Coverage Δ
...va/org/apache/uniffle/client/util/ClientUtils.java 21.05% <50.00%> (+2.87%) ⬆️
...preduce/task/reduce/RssRemoteMergeManagerImpl.java
.../java/org/apache/hadoop/mapreduce/RssMRConfig.java
...pache/hadoop/mapreduce/task/reduce/RssShuffle.java
...java/org/apache/hadoop/mapred/SortWriteBuffer.java
...n/java/org/apache/hadoop/mapreduce/MRIdHelper.java
.../hadoop/mapreduce/task/reduce/RssBypassWriter.java
...rg/apache/hadoop/mapred/RssMapOutputCollector.java
...pache/hadoop/mapreduce/task/reduce/RssFetcher.java
...apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java
... and 4 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kaijchen kaijchen requested review from duanmeng and jerqi November 2, 2022 09:20
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RssException may be better. Because we will collect the RssException to judge the failure apps caused by RSS.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jerqi
Copy link
Contributor

jerqi commented Nov 3, 2022

fetchRemoteStorage is used for spark2, spark3 and mr.

@jerqi jerqi self-requested a review November 4, 2022 08:52
Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @kaijchen

@jerqi jerqi merged commit 6b65bbb into apache:master Nov 11, 2022
@kaijchen kaijchen deleted the cleanup-client-utils branch November 11, 2022 02:18
@kaijchen
Copy link
Member Author

Thanks @jerqi for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants