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/02/01 08:18:50 UTC

[GitHub] [incubator-uniffle] kaijchen opened a new pull request, #537: Fix error handling in LocalFileWriteHandler#createBasePath()

kaijchen opened a new pull request, #537:
URL: https://github.com/apache/incubator-uniffle/pull/537

   ### What changes were proposed in this pull request?
   
   Fix error handling in `LocalFileWriteHandler#createBasePath()`.
   Throw an `RuntimeException` in case of failure.
   
   Note: raw use of `RuntimeException` is not a good practice.
   But in this case, it will be propagated to a lambda. 
   
   ### Why are the changes needed?
   
   The return value of `baseFolder.mkdirs()` was ignored.
   
   ### 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] advancedxy commented on a diff in pull request #537: Throw exception when failed to create shuffle base dir

Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #537:
URL: https://github.com/apache/incubator-uniffle/pull/537#discussion_r1093958789


##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -51,19 +53,10 @@ 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 (SecurityException e) {
-        // if folder exist, ignore the exception
-        if (!baseFolder.exists()) {
-          LOG.error("Can't create shuffle folder:" + basePath, e);
-          throw e;
-        }
-      }
+    try {
+      Files.createDirectories(Paths.get(basePath));

Review Comment:
   Yeah. but there's q shortcut, if the dir is already existed, we don't have to invoke `createDirectories`, which seems quite complex compared to check the existence of dir.



-- 
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 #537: Fix error handling in LocalFileWriteHandler#createBasePath()

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #537:
URL: https://github.com/apache/incubator-uniffle/pull/537#issuecomment-1411670666

   # [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/537?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 [#537](https://codecov.io/gh/apache/incubator-uniffle/pull/537?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (663001f) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/721b22cf0a02c0d3fdb65851aa1edd8271dbe138?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (721b22c) will **increase** coverage by `4.08%`.
   > The diff coverage is `33.33%`.
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #537      +/-   ##
   ============================================
   + Coverage     60.23%   64.31%   +4.08%     
   + Complexity     1785     1652     -133     
   ============================================
     Files           205      186      -19     
     Lines         11557     9114    -2443     
     Branches       1042      918     -124     
   ============================================
   - Hits           6961     5862    -1099     
   + Misses         4189     2936    -1253     
   + Partials        407      316      -91     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...le/storage/handler/impl/LocalFileWriteHandler.java](https://codecov.io/gh/apache/incubator-uniffle/pull/537?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==) | `80.00% <33.33%> (+4.48%)` | :arrow_up: |
   | [...ategy/storage/AppBalanceSelectStorageStrategy.java](https://codecov.io/gh/apache/incubator-uniffle/pull/537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3Ivc3RyYXRlZ3kvc3RvcmFnZS9BcHBCYWxhbmNlU2VsZWN0U3RvcmFnZVN0cmF0ZWd5LmphdmE=) | `72.00% <0.00%> (-4.00%)` | :arrow_down: |
   | [.../hadoop/mapreduce/task/reduce/RssBypassWriter.java](https://codecov.io/gh/apache/incubator-uniffle/pull/537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL3Rhc2svcmVkdWNlL1Jzc0J5cGFzc1dyaXRlci5qYXZh) | | |
   | [.../hadoop/mapreduce/task/reduce/RssEventFetcher.java](https://codecov.io/gh/apache/incubator-uniffle/pull/537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL3Rhc2svcmVkdWNlL1Jzc0V2ZW50RmV0Y2hlci5qYXZh) | | |
   | [...oy/kubernetes/operator/pkg/controller/util/util.go](https://codecov.io/gh/apache/incubator-uniffle/pull/537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGVwbG95L2t1YmVybmV0ZXMvb3BlcmF0b3IvcGtnL2NvbnRyb2xsZXIvdXRpbC91dGlsLmdv) | | |
   | [...n/java/org/apache/hadoop/mapreduce/RssMRUtils.java](https://codecov.io/gh/apache/incubator-uniffle/pull/537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL1Jzc01SVXRpbHMuamF2YQ==) | | |
   | [...rg/apache/hadoop/mapred/RssMapOutputCollector.java](https://codecov.io/gh/apache/incubator-uniffle/pull/537?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) | | |
   | [...n/java/org/apache/hadoop/mapreduce/MRIdHelper.java](https://codecov.io/gh/apache/incubator-uniffle/pull/537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL01SSWRIZWxwZXIuamF2YQ==) | | |
   | [...java/org/apache/hadoop/mapred/SortWriteBuffer.java](https://codecov.io/gh/apache/incubator-uniffle/pull/537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkL1NvcnRXcml0ZUJ1ZmZlci5qYXZh) | | |
   | [...y/kubernetes/operator/pkg/webhook/inspector/rss.go](https://codecov.io/gh/apache/incubator-uniffle/pull/537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGVwbG95L2t1YmVybmV0ZXMvb3BlcmF0b3IvcGtnL3dlYmhvb2svaW5zcGVjdG9yL3Jzcy5nbw==) | | |
   | ... and [12 more](https://codecov.io/gh/apache/incubator-uniffle/pull/537?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 #537: Throw exception when failed to create shuffle base dir

Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #537:
URL: https://github.com/apache/incubator-uniffle/pull/537#discussion_r1094072713


##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -51,19 +53,10 @@ 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 (SecurityException e) {
-        // if folder exist, ignore the exception
-        if (!baseFolder.exists()) {
-          LOG.error("Can't create shuffle folder:" + basePath, e);
-          throw e;
-        }
-      }
+    try {
+      Files.createDirectories(Paths.get(basePath));

Review Comment:
   I simply mean:
   ```
   if (!file.isDirectoyr) {
     try {
       Files.createDirectories(path);
     } catch {...}
   }
   ```



-- 
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 #537: Throw exception when failed to create shuffle base dir

Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #537:
URL: https://github.com/apache/incubator-uniffle/pull/537#discussion_r1093256221


##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -51,19 +53,10 @@ 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 (SecurityException e) {
-        // if folder exist, ignore the exception
-        if (!baseFolder.exists()) {
-          LOG.error("Can't create shuffle folder:" + basePath, e);
-          throw e;
-        }
-      }
+    try {
+      Files.createDirectories(Paths.get(basePath));

Review Comment:
   `createDirectories` looks quite complex.
   
   let's shortcut this operator by check the basePath has already been existed or not? 



-- 
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 #537: Result of mkdirs() is ignored in LocalFileWriteHandler#createBasePath()

Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #537:
URL: https://github.com/apache/incubator-uniffle/pull/537#discussion_r1094100150


##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -51,19 +53,10 @@ 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 (SecurityException e) {
-        // if folder exist, ignore the exception
-        if (!baseFolder.exists()) {
-          LOG.error("Can't create shuffle folder:" + basePath, e);
-          throw e;
-        }
-      }
+    try {
+      Files.createDirectories(Paths.get(basePath));

Review Comment:
   OK, I got it.



-- 
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 #537: Throw exception when failed to create shuffle base dir

Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #537:
URL: https://github.com/apache/incubator-uniffle/pull/537#discussion_r1093259423


##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -51,19 +53,10 @@ 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 (SecurityException e) {
-        // if folder exist, ignore the exception
-        if (!baseFolder.exists()) {
-          LOG.error("Can't create shuffle folder:" + basePath, e);
-          throw e;
-        }
-      }
+    try {
+      Files.createDirectories(Paths.get(basePath));

Review Comment:
   > Creates a directory by creating all nonexistent parent directories first. Unlike the createDirectory method, an exception is not thrown if the directory could not be created because it already exists.
   
   Isn't it exactly what we want?
   



-- 
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 #537: Result of mkdirs() is ignored in LocalFileWriteHandler#createBasePath()

Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen merged PR #537:
URL: https://github.com/apache/incubator-uniffle/pull/537


-- 
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 #537: Throw exception when failed to create shuffle base dir

Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #537:
URL: https://github.com/apache/incubator-uniffle/pull/537#discussion_r1093978769


##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -51,19 +53,10 @@ 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 (SecurityException e) {
-        // if folder exist, ignore the exception
-        if (!baseFolder.exists()) {
-          LOG.error("Can't create shuffle folder:" + basePath, e);
-          throw e;
-        }
-      }
+    try {
+      Files.createDirectories(Paths.get(basePath));

Review Comment:
   > If there will be a significant performance difference
   
   I don't mean there's a big performance, but it's slightly more complex than just exist its existence.
   
   >Even if it can improve the case when the dir already exists, it will worsen the case when the dir doesn't exist.
   
   This base dir should probably already be created.
   



-- 
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 #537: Throw exception when failed to create shuffle base dir

Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #537:
URL: https://github.com/apache/incubator-uniffle/pull/537#discussion_r1093988699


##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -51,19 +53,10 @@ 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 (SecurityException e) {
-        // if folder exist, ignore the exception
-        if (!baseFolder.exists()) {
-          LOG.error("Can't create shuffle folder:" + basePath, e);
-          throw e;
-        }
-      }
+    try {
+      Files.createDirectories(Paths.get(basePath));

Review Comment:
   We will have to check existence and throw exception when a file (not a directory) exists for each parent directory.
   Which is complex and repeated work since it's already done in `Files.createDirectories()`.



-- 
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 #537: Throw exception when failed to create shuffle base dir

Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #537:
URL: https://github.com/apache/incubator-uniffle/pull/537#discussion_r1093964688


##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -51,19 +53,10 @@ 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 (SecurityException e) {
-        // if folder exist, ignore the exception
-        if (!baseFolder.exists()) {
-          LOG.error("Can't create shuffle folder:" + basePath, e);
-          throw e;
-        }
-      }
+    try {
+      Files.createDirectories(Paths.get(basePath));

Review Comment:
   > Yeah. but there's q shortcut, if the dir is already existed, we don't have to invoke `createDirectories`, which seems quite complex compared to check the existence of dir.
   
   If there will be a significant performance difference, why doesn't the library author do this in the `createDirectories`?
   Even if it can improve the case when the dir already exists, it will worsen the case when the dir doesn't exist.



-- 
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 #537: Result of mkdirs() is ignored in LocalFileWriteHandler#createBasePath()

Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on PR #537:
URL: https://github.com/apache/incubator-uniffle/pull/537#issuecomment-1413398513

   Thanks @advancedxy and @zuston 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] zuston commented on a diff in pull request #537: Throw exception when failed to create shuffle base dir

Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #537:
URL: https://github.com/apache/incubator-uniffle/pull/537#discussion_r1094077879


##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileWriteHandler.java:
##########
@@ -51,19 +53,10 @@ 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 (SecurityException e) {
-        // if folder exist, ignore the exception
-        if (!baseFolder.exists()) {
-          LOG.error("Can't create shuffle folder:" + basePath, e);
-          throw e;
-        }
-      }
+    try {
+      Files.createDirectories(Paths.get(basePath));
+    } catch (IOException e) {
+      throw new RuntimeException("Failed to create shuffle folder: " + basePath, e);

Review Comment:
   How about `RssException`  ?



-- 
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