You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "kaijchen (via GitHub)" <gi...@apache.org> on 2023/01/31 07:55:49 UTC
[GitHub] [incubator-uniffle] kaijchen opened a new pull request, #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
kaijchen opened a new pull request, #527:
URL: https://github.com/apache/incubator-uniffle/pull/527
### What changes were proposed in this pull request?
1. Do not catch Exception when Exception is not thrown within the try block, and RuntimeException is not explicitly caught.
2. Remove REC_CATCH_EXCEPTION from SpotBugs exclusion.
### Why are the changes needed?
Fix [REC_CATCH_EXCEPTION][1].
[1]: https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#rec-exception-is-caught-when-exception-is-not-thrown-rec-catch-exception
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#discussion_r1092695172
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -52,17 +52,11 @@ public LocalFileWriteHandler(
private void createBasePath() {
File baseFolder = new File(basePath);
- // check if shuffle folder exist
- if (!baseFolder.exists()) {
- try {
- // try to create folder, it may be created by other Shuffle Server
- baseFolder.mkdirs();
- } catch (Exception e) {
- // if folder exist, ignore the exception
- if (!baseFolder.exists()) {
- LOG.error("Can't create shuffle folder:" + basePath, e);
- throw e;
- }
+ // try to create folder, it may already exist
+ if (!baseFolder.isDirectory() && !baseFolder.mkdirs()) {
+ // if folder is not created successfully, make sure it exists
+ if (!baseFolder.isDirectory()) {
+ LOG.error("Can't create shuffle folder: {}", basePath);
Review Comment:
After a second thought, let's just focus on spotbug issues in this PR.
The `createBasePath()` can be fixed in another PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#discussion_r1091832704
##########
client-mr/src/main/java/org/apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java:
##########
@@ -366,7 +366,10 @@ static void writeExtraConf(JobConf conf, JobConf extraConf) {
long size = status.getLen();
String sizes = conf.get(MRJobConfig.CACHE_FILES_SIZES);
conf.set(MRJobConfig.CACHE_FILES_SIZES, sizes == null ? String.valueOf(size) : size + "," + sizes);
- } catch (Exception e) {
+ } catch (InterruptedException e) {
+ LOG.error("Interrupted while trying to upload extra conf ", e);
+ Thread.currentThread().interrupt();
Review Comment:
https://stackoverflow.com/questions/3976344/handling-interruptedexception-in-java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#discussion_r1091852773
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -52,17 +52,11 @@ public LocalFileWriteHandler(
private void createBasePath() {
File baseFolder = new File(basePath);
- // check if shuffle folder exist
- if (!baseFolder.exists()) {
Review Comment:
`!baseFolder.exists()` does not mean `baseFolder.isDirectory()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#discussion_r1091826852
##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java:
##########
@@ -528,7 +529,7 @@ private Roaring64NavigableMap getExpectedTasksByExecutorId(
endMapIndex,
startPartition,
endPartition);
- } catch (Exception ignored) {
+ } catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException ignored1) {
Review Comment:
This is expected to ignore all the exceptions including RuntimeException.
Let's use annotation to suppress the warning then?
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -52,17 +52,11 @@ public LocalFileWriteHandler(
private void createBasePath() {
File baseFolder = new File(basePath);
- // check if shuffle folder exist
- if (!baseFolder.exists()) {
- try {
- // try to create folder, it may be created by other Shuffle Server
- baseFolder.mkdirs();
- } catch (Exception e) {
- // if folder exist, ignore the exception
- if (!baseFolder.exists()) {
- LOG.error("Can't create shuffle folder:" + basePath, e);
- throw e;
- }
+ // try to create folder, it may already exist
+ if (!baseFolder.mkdirs()) {
Review Comment:
this doesn't seem right.
`mkdirs` may throw exceptions. you have to raise the exception to sync with old logic.
##########
client-mr/src/main/java/org/apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java:
##########
@@ -366,7 +366,10 @@ static void writeExtraConf(JobConf conf, JobConf extraConf) {
long size = status.getLen();
String sizes = conf.get(MRJobConfig.CACHE_FILES_SIZES);
conf.set(MRJobConfig.CACHE_FILES_SIZES, sizes == null ? String.valueOf(size) : size + "," + sizes);
- } catch (Exception e) {
+ } catch (InterruptedException e) {
+ LOG.error("Interrupted while trying to upload extra conf ", e);
+ Thread.currentThread().interrupt();
Review Comment:
this is not right.
`InterruptedException` means this thread has already been interrupted, you cannot interrupt current thread then.
Let's catch the `RuntimeException` first, then the normal wildcard Exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#discussion_r1092691206
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -52,17 +52,11 @@ public LocalFileWriteHandler(
private void createBasePath() {
File baseFolder = new File(basePath);
- // check if shuffle folder exist
- if (!baseFolder.exists()) {
- try {
- // try to create folder, it may be created by other Shuffle Server
- baseFolder.mkdirs();
- } catch (Exception e) {
- // if folder exist, ignore the exception
- if (!baseFolder.exists()) {
- LOG.error("Can't create shuffle folder:" + basePath, e);
- throw e;
- }
+ // try to create folder, it may already exist
+ if (!baseFolder.isDirectory() && !baseFolder.mkdirs()) {
+ // if folder is not created successfully, make sure it exists
+ if (!baseFolder.isDirectory()) {
+ LOG.error("Can't create shuffle folder: {}", basePath);
Review Comment:
the logging should be more verbose about whether this is a file or not.
Another things is that when basePath could not be created, a runtime exception should be raised.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#discussion_r1091868738
##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java:
##########
@@ -528,7 +529,7 @@ private Roaring64NavigableMap getExpectedTasksByExecutorId(
endMapIndex,
startPartition,
endPartition);
- } catch (Exception ignored) {
+ } catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException ignored1) {
Review Comment:
No. NPE should still need to be caught as it's followed by an fallback.
Any exception raised should fallback to the second attempt.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#discussion_r1091839317
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -52,17 +52,11 @@ public LocalFileWriteHandler(
private void createBasePath() {
File baseFolder = new File(basePath);
- // check if shuffle folder exist
- if (!baseFolder.exists()) {
- try {
- // try to create folder, it may be created by other Shuffle Server
- baseFolder.mkdirs();
- } catch (Exception e) {
- // if folder exist, ignore the exception
- if (!baseFolder.exists()) {
- LOG.error("Can't create shuffle folder:" + basePath, e);
- throw e;
- }
+ // try to create folder, it may already exist
+ if (!baseFolder.mkdirs()) {
Review Comment:
The old logic does not match with the comments.
`File#mkdir()` and `File#exists()` both throws `SecurityException`, which is a `RuntimeException`.
However, the permission required is different, maybe we should check `File#exists()` first as it only requires read permission.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#issuecomment-1410214208
Cloud you add an umbrella issue to track all the spotbugs improvements?
You may don't have to create sub-issues for each pattern, but it's good to tracking in the improvement in the umbrella issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] kaijchen merged pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen merged PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#discussion_r1091830274
##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java:
##########
@@ -528,7 +529,7 @@ private Roaring64NavigableMap getExpectedTasksByExecutorId(
endMapIndex,
startPartition,
endPartition);
- } catch (Exception ignored) {
+ } catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException ignored1) {
Review Comment:
I believe some RuntimeException such as NPE should not be catched.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#discussion_r1091887844
##########
client-mr/src/main/java/org/apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java:
##########
@@ -366,7 +366,10 @@ static void writeExtraConf(JobConf conf, JobConf extraConf) {
long size = status.getLen();
String sizes = conf.get(MRJobConfig.CACHE_FILES_SIZES);
conf.set(MRJobConfig.CACHE_FILES_SIZES, sizes == null ? String.valueOf(size) : size + "," + sizes);
- } catch (Exception e) {
+ } catch (InterruptedException e) {
+ LOG.error("Interrupted while trying to upload extra conf ", e);
+ Thread.currentThread().interrupt();
Review Comment:
Still no.
For this method, an interrupt exception means this method doesn't complete succeeded. The caller method should abort rather than continues executing.
You either to propagate this InterruptedException or wrapped it into another runtime exception,
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#discussion_r1091955123
##########
client-mr/src/main/java/org/apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java:
##########
@@ -366,7 +366,10 @@ static void writeExtraConf(JobConf conf, JobConf extraConf) {
long size = status.getLen();
String sizes = conf.get(MRJobConfig.CACHE_FILES_SIZES);
conf.set(MRJobConfig.CACHE_FILES_SIZES, sizes == null ? String.valueOf(size) : size + "," + sizes);
- } catch (Exception e) {
+ } catch (InterruptedException e) {
+ LOG.error("Interrupted while trying to upload extra conf ", e);
+ Thread.currentThread().interrupt();
Review Comment:
> By now it should be clear that just doing throw new RuntimeException(e) is a bad idea. It isn't very polite to the caller. You could invent a new runtime exception but the root cause (someone wants the thread to stop execution) might get lost.
Quoted from the link above, it's a bad idea to throw new RuntimeException(e).
And we cannot propagate InterruptedException here because `RssMRAppMaster` extends `org.apache.hadoop.mapreduce.v2.app.MRAppMaster`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] kaijchen commented on pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#issuecomment-1411603196
Thanks @advancedxy for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#discussion_r1091967114
##########
client-mr/src/main/java/org/apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java:
##########
@@ -366,7 +366,10 @@ static void writeExtraConf(JobConf conf, JobConf extraConf) {
long size = status.getLen();
String sizes = conf.get(MRJobConfig.CACHE_FILES_SIZES);
conf.set(MRJobConfig.CACHE_FILES_SIZES, sizes == null ? String.valueOf(size) : size + "," + sizes);
- } catch (Exception e) {
+ } catch (InterruptedException e) {
+ LOG.error("Interrupted while trying to upload extra conf ", e);
+ Thread.currentThread().interrupt();
Review Comment:
We could check `Thread.currentThread().isInterrupted()` in `main()` or just call `System.exit()` here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] codecov-commenter commented on pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#issuecomment-1409981458
# [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/527?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#527](https://codecov.io/gh/apache/incubator-uniffle/pull/527?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1d94a28) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/6367b36e0b94fbd7d70d5489168a53960a8ae31c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6367b36) will **increase** coverage by `4.04%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #527 +/- ##
============================================
+ Coverage 60.11% 64.15% +4.04%
+ Complexity 1769 1636 -133
============================================
Files 205 186 -19
Lines 11532 9089 -2443
Branches 1033 911 -122
============================================
- Hits 6932 5831 -1101
+ Misses 4194 2942 -1252
+ Partials 406 316 -90
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/527?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../org/apache/uniffle/common/config/ConfigUtils.java](https://codecov.io/gh/apache/incubator-uniffle/pull/527?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9jb25maWcvQ29uZmlnVXRpbHMuamF2YQ==) | `95.55% <0.00%> (ø)` | |
| [.../apache/uniffle/coordinator/ClientConfManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/527?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvQ2xpZW50Q29uZk1hbmFnZXIuamF2YQ==) | `92.95% <0.00%> (ø)` | |
| [...inator/access/checker/AccessCandidatesChecker.java](https://codecov.io/gh/apache/incubator-uniffle/pull/527?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvYWNjZXNzL2NoZWNrZXIvQWNjZXNzQ2FuZGlkYXRlc0NoZWNrZXIuamF2YQ==) | `85.50% <0.00%> (ø)` | |
| [.../storage/handler/impl/HdfsShuffleWriteHandler.java](https://codecov.io/gh/apache/incubator-uniffle/pull/527?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmFnZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvdW5pZmZsZS9zdG9yYWdlL2hhbmRsZXIvaW1wbC9IZGZzU2h1ZmZsZVdyaXRlSGFuZGxlci5qYXZh) | `87.09% <0.00%> (ø)` | |
| [...le/storage/handler/impl/LocalFileWriteHandler.java](https://codecov.io/gh/apache/incubator-uniffle/pull/527?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmFnZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvdW5pZmZsZS9zdG9yYWdlL2hhbmRsZXIvaW1wbC9Mb2NhbEZpbGVXcml0ZUhhbmRsZXIuamF2YQ==) | `77.77% <0.00%> (+2.26%)` | :arrow_up: |
| [...rg/apache/uniffle/storage/common/LocalStorage.java](https://codecov.io/gh/apache/incubator-uniffle/pull/527?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmFnZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvdW5pZmZsZS9zdG9yYWdlL2NvbW1vbi9Mb2NhbFN0b3JhZ2UuamF2YQ==) | `48.36% <0.00%> (-1.97%)` | :arrow_down: |
| [...apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java](https://codecov.io/gh/apache/incubator-uniffle/pull/527?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL3YyL2FwcC9Sc3NNUkFwcE1hc3Rlci5qYXZh) | | |
| [...rnetes/operator/pkg/webhook/inspector/inspector.go](https://codecov.io/gh/apache/incubator-uniffle/pull/527?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGVwbG95L2t1YmVybmV0ZXMvb3BlcmF0b3IvcGtnL3dlYmhvb2svaW5zcGVjdG9yL2luc3BlY3Rvci5nbw==) | | |
| [...rg/apache/hadoop/mapred/RssMapOutputCollector.java](https://codecov.io/gh/apache/incubator-uniffle/pull/527?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkL1Jzc01hcE91dHB1dENvbGxlY3Rvci5qYXZh) | | |
| ... and [16 more](https://codecov.io/gh/apache/incubator-uniffle/pull/527?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#discussion_r1092854430
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -52,17 +52,11 @@ public LocalFileWriteHandler(
private void createBasePath() {
File baseFolder = new File(basePath);
- // check if shuffle folder exist
- if (!baseFolder.exists()) {
- try {
- // try to create folder, it may be created by other Shuffle Server
- baseFolder.mkdirs();
- } catch (Exception e) {
- // if folder exist, ignore the exception
- if (!baseFolder.exists()) {
- LOG.error("Can't create shuffle folder:" + basePath, e);
- throw e;
- }
+ // try to create folder, it may already exist
+ if (!baseFolder.isDirectory() && !baseFolder.mkdirs()) {
+ // if folder is not created successfully, make sure it exists
+ if (!baseFolder.isDirectory()) {
+ LOG.error("Can't create shuffle folder: {}", basePath);
Review Comment:
SGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#discussion_r1091852773
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -52,17 +52,11 @@ public LocalFileWriteHandler(
private void createBasePath() {
File baseFolder = new File(basePath);
- // check if shuffle folder exist
- if (!baseFolder.exists()) {
Review Comment:
`!baseFolder.exists()` does not mean `baseFolder.isDirectory()`.
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -52,17 +52,11 @@ public LocalFileWriteHandler(
private void createBasePath() {
File baseFolder = new File(basePath);
- // check if shuffle folder exist
- if (!baseFolder.exists()) {
- try {
- // try to create folder, it may be created by other Shuffle Server
- baseFolder.mkdirs();
- } catch (Exception e) {
Review Comment:
Only `SecurityException` will be thrown when access is denied by `System.getSecurityManager()`.
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -52,17 +52,11 @@ public LocalFileWriteHandler(
private void createBasePath() {
File baseFolder = new File(basePath);
- // check if shuffle folder exist
- if (!baseFolder.exists()) {
- try {
- // try to create folder, it may be created by other Shuffle Server
- baseFolder.mkdirs();
Review Comment:
Return value is ignored: `false` means failure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#discussion_r1091871878
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -52,17 +52,11 @@ public LocalFileWriteHandler(
private void createBasePath() {
File baseFolder = new File(basePath);
- // check if shuffle folder exist
- if (!baseFolder.exists()) {
- try {
- // try to create folder, it may be created by other Shuffle Server
- baseFolder.mkdirs();
- } catch (Exception e) {
- // if folder exist, ignore the exception
- if (!baseFolder.exists()) {
- LOG.error("Can't create shuffle folder:" + basePath, e);
- throw e;
- }
+ // try to create folder, it may already exist
+ if (!baseFolder.mkdirs()) {
Review Comment:
Yes. You need to check `exists` first, then try to `mkdir`. if it doesn't succeed or throws an security exception. The corresponding exception should be thrown.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#discussion_r1091967114
##########
client-mr/src/main/java/org/apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java:
##########
@@ -366,7 +366,10 @@ static void writeExtraConf(JobConf conf, JobConf extraConf) {
long size = status.getLen();
String sizes = conf.get(MRJobConfig.CACHE_FILES_SIZES);
conf.set(MRJobConfig.CACHE_FILES_SIZES, sizes == null ? String.valueOf(size) : size + "," + sizes);
- } catch (Exception e) {
+ } catch (InterruptedException e) {
+ LOG.error("Interrupted while trying to upload extra conf ", e);
+ Thread.currentThread().interrupt();
Review Comment:
We could check `Thread.currentThread().isInterrupted()` in `main()` or just call `System.exit()` here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#discussion_r1092116775
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -52,17 +52,11 @@ public LocalFileWriteHandler(
private void createBasePath() {
File baseFolder = new File(basePath);
- // check if shuffle folder exist
- if (!baseFolder.exists()) {
- try {
- // try to create folder, it may be created by other Shuffle Server
- baseFolder.mkdirs();
- } catch (Exception e) {
- // if folder exist, ignore the exception
- if (!baseFolder.exists()) {
- LOG.error("Can't create shuffle folder:" + basePath, e);
- throw e;
- }
+ // try to create folder, it may already exist
+ if (!baseFolder.mkdirs()) {
Review Comment:
> ... if it doesn't succeed or throws an security exception. The corresponding exception should be thrown.
I'm not catching any exception here, SecurityException will be thrown when SecurityManager denied the operation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #527: [SpotBugs] Fix REC_CATCH_EXCEPTION
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #527:
URL: https://github.com/apache/incubator-uniffle/pull/527#discussion_r1092689261
##########
client-mr/src/main/java/org/apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java:
##########
@@ -366,7 +366,10 @@ static void writeExtraConf(JobConf conf, JobConf extraConf) {
long size = status.getLen();
String sizes = conf.get(MRJobConfig.CACHE_FILES_SIZES);
conf.set(MRJobConfig.CACHE_FILES_SIZES, sizes == null ? String.valueOf(size) : size + "," + sizes);
- } catch (Exception e) {
+ } catch (InterruptedException e) {
+ LOG.error("Interrupted while trying to upload extra conf ", e);
+ Thread.currentThread().interrupt();
Review Comment:
The quote should be considered case by case, It applies to most cases. For this case, see how the InterruptedException is introduced itself by `Cluster#getFileSystem()`
https://github.com/apache/hadoop/blob/6d325d9d09cf8595e1e39e39128abd98dea75220/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/Cluster.java#L189-L200
It actually swallows InterruptedException.
I think it's reasonable to just wrap it here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org