You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "advancedxy (via GitHub)" <gi...@apache.org> on 2023/02/16 11:26:45 UTC
[GitHub] [incubator-uniffle] advancedxy opened a new pull request, #616: [#571][FOLLOWUP] don't recreate base dir if it's already an dir
advancedxy opened a new pull request, #616:
URL: https://github.com/apache/incubator-uniffle/pull/616
### What changes were proposed in this pull request?
skip recreate base dir if it's already an dir
### Why are the changes needed?
handles some corner cases such as the base dir is an mount point root path.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing UTs.
--
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 #616: [#571][FOLLOWUP] fix: don't recreate base dir if it's already an dir
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #616:
URL: https://github.com/apache/incubator-uniffle/pull/616#discussion_r1109235399
##########
storage/src/main/java/org/apache/uniffle/storage/common/LocalStorage.java:
##########
@@ -81,9 +81,16 @@ private LocalStorage(Builder builder) {
if (isEmptyAndWritableDir(baseFolder)) {
LOG.warn("Base directory is already an empty dir, skip init");
} else {
- FileUtils.deleteDirectory(baseFolder);
- if (!baseFolder.mkdirs()) {
- throw new IOException("Failed to create base folder: " + basePath);
+ // base folder could be the mounted path, which could not be deleted.
+ // just clean the directory instead.
+ if (baseFolder.isDirectory()) {
+ LOG.info("Base directory is already existed, just clean the directory");
+ FileUtils.cleanDirectory(baseFolder);
+ } else {
+ FileUtils.deleteDirectory(baseFolder);
+ if (!baseFolder.mkdirs()) {
+ throw new IOException("Failed to create base folder: " + basePath);
+ }
}
}
Review Comment:
I mean this
```java
} else if (baseFolder.isDirectory()) {
// base folder could be the mounted path, which could not be deleted.
// just clean the directory instead.
LOG.info("Base directory is already existed, just clean the directory");
FileUtils.cleanDirectory(baseFolder);
} else {
FileUtils.deleteDirectory(baseFolder);
if (!baseFolder.mkdirs()) {
throw new IOException("Failed to create base folder: " + basePath);
}
}
```
##########
storage/src/main/java/org/apache/uniffle/storage/common/LocalStorage.java:
##########
@@ -81,9 +81,16 @@ private LocalStorage(Builder builder) {
if (isEmptyAndWritableDir(baseFolder)) {
LOG.warn("Base directory is already an empty dir, skip init");
} else {
- FileUtils.deleteDirectory(baseFolder);
- if (!baseFolder.mkdirs()) {
- throw new IOException("Failed to create base folder: " + basePath);
+ // base folder could be the mounted path, which could not be deleted.
+ // just clean the directory instead.
+ if (baseFolder.isDirectory()) {
+ LOG.info("Base directory is already existed, just clean the directory");
+ FileUtils.cleanDirectory(baseFolder);
+ } else {
+ FileUtils.deleteDirectory(baseFolder);
Review Comment:
Also, why call `FileUtils.deleteDirectory` if `baseFolder` is not a directory?
--
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 #616: [#571][FOLLOWUP] don't recreate base dir if it's already an dir
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #616:
URL: https://github.com/apache/incubator-uniffle/pull/616#discussion_r1108681090
##########
storage/src/main/java/org/apache/uniffle/storage/common/LocalStorage.java:
##########
@@ -81,9 +81,16 @@ private LocalStorage(Builder builder) {
if (isEmptyAndWritableDir(baseFolder)) {
LOG.warn("Base directory is already an empty dir, skip init");
} else {
- FileUtils.deleteDirectory(baseFolder);
- if (!baseFolder.mkdirs()) {
- throw new IOException("Failed to create base folder: " + basePath);
+ // base folder could be the mounted path, which could not be deleted.
+ // just clean the directory instead.
+ if (baseFolder.isDirectory()) {
+ LOG.warn("Base directory is already existed, just clean");
Review Comment:
Is it necessary to use `warn` here? I think `info` should be enough.
--
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] jerqi commented on pull request #616: [#571][FOLLOWUP] fix: don't recreate base dir if it's already an dir
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #616:
URL: https://github.com/apache/incubator-uniffle/pull/616#issuecomment-1434042793
cc @zuston
--
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 #616: [#571][FOLLOWUP] fix: don't recreate base dir if it's already an dir
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #616:
URL: https://github.com/apache/incubator-uniffle/pull/616#discussion_r1109262347
##########
storage/src/main/java/org/apache/uniffle/storage/common/LocalStorage.java:
##########
@@ -78,9 +78,15 @@ private LocalStorage(Builder builder) {
File baseFolder = new File(basePath);
try {
- if (isEmptyAndWritableDir(baseFolder)) {
- LOG.warn("Base directory is already an empty dir, skip init");
+ // base folder could be the mounted path, which could not be deleted.
+ // just clean the directory instead.
+ if (baseFolder.isDirectory()) {
+ LOG.info("Base directory is already existed, just clean the directory");
+ FileUtils.cleanDirectory(baseFolder);
} else {
+ // base folder could be not existed or a regular file instead of dir.
+ // just try to deleteDirectory and recreate it. `deleteDirectory` would throw an exception
+ // when the base folder is a file.
FileUtils.deleteDirectory(baseFolder);
Review Comment:
Is it still necessary? Because we already know `baseFolder.isDirectory() == false`.
Although one possible case is `baseFolder` being a symlink to a directory.
--
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] jerqi commented on pull request #616: [#571][FOLLOWUP] fix: don't recreate base dir if it's already an dir
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #616:
URL: https://github.com/apache/incubator-uniffle/pull/616#issuecomment-1434001758
Should this pr be included in version 0.7?
--
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 #616: [#571][FOLLOWUP] fix: optimize base dir init process
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #616:
URL: https://github.com/apache/incubator-uniffle/pull/616#issuecomment-1434156273
> > If this pr is merged, we should backport this pr to 0.7.
>
> It's OK for me.
thanks @zuston, I will raise PR to branch-0.7, once this is merged.
--
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 #616: [#571][FOLLOWUP] fix: don't recreate base dir if it's already an dir
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #616:
URL: https://github.com/apache/incubator-uniffle/pull/616#discussion_r1109235399
##########
storage/src/main/java/org/apache/uniffle/storage/common/LocalStorage.java:
##########
@@ -81,9 +81,16 @@ private LocalStorage(Builder builder) {
if (isEmptyAndWritableDir(baseFolder)) {
LOG.warn("Base directory is already an empty dir, skip init");
} else {
- FileUtils.deleteDirectory(baseFolder);
- if (!baseFolder.mkdirs()) {
- throw new IOException("Failed to create base folder: " + basePath);
+ // base folder could be the mounted path, which could not be deleted.
+ // just clean the directory instead.
+ if (baseFolder.isDirectory()) {
+ LOG.info("Base directory is already existed, just clean the directory");
+ FileUtils.cleanDirectory(baseFolder);
+ } else {
+ FileUtils.deleteDirectory(baseFolder);
+ if (!baseFolder.mkdirs()) {
+ throw new IOException("Failed to create base folder: " + basePath);
+ }
}
}
Review Comment:
I mean this
```java
} else if (baseFolder.isDirectory()) {
// base folder could be the mounted path, which could not be deleted.
// just clean the directory instead.
LOG.info("Base directory is already existed, just clean the directory");
FileUtils.cleanDirectory(baseFolder);
} else {
FileUtils.deleteDirectory(baseFolder);
if (!baseFolder.mkdirs()) {
throw new IOException("Failed to create base folder: " + basePath);
}
}
```
--
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 #616: [#571][FOLLOWUP] don't recreate base dir if it's already an dir
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on PR #616:
URL: https://github.com/apache/incubator-uniffle/pull/616#issuecomment-1433304395
> The diff coverage is `33.33%`.
To my surprise, the else branch is not covered.
<img width="811" alt="image" src="https://user-images.githubusercontent.com/5821159/219417074-928ea729-dbd4-4d38-9267-4bd005d86bda.png">
--
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 #616: [#571][FOLLOWUP] don't recreate base dir if it's already an dir
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #616:
URL: https://github.com/apache/incubator-uniffle/pull/616#issuecomment-1433208436
> LGTM, thanks @advancedxy. Can you add a test to cover your change?
Let me check that. This behavior should already been covered by existing UTs.
--
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 #616: [#571][FOLLOWUP] fix: don't recreate base dir if it's already an dir
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #616:
URL: https://github.com/apache/incubator-uniffle/pull/616#discussion_r1109230594
##########
storage/src/main/java/org/apache/uniffle/storage/common/LocalStorage.java:
##########
@@ -81,9 +81,16 @@ private LocalStorage(Builder builder) {
if (isEmptyAndWritableDir(baseFolder)) {
LOG.warn("Base directory is already an empty dir, skip init");
} else {
- FileUtils.deleteDirectory(baseFolder);
- if (!baseFolder.mkdirs()) {
- throw new IOException("Failed to create base folder: " + basePath);
+ // base folder could be the mounted path, which could not be deleted.
+ // just clean the directory instead.
+ if (baseFolder.isDirectory()) {
Review Comment:
Please see the latest commit.
--
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 pull request #616: [#571][FOLLOWUP] fix: don't recreate base dir if it's already an dir
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #616:
URL: https://github.com/apache/incubator-uniffle/pull/616#issuecomment-1434043353
> If this pr is merged, we should backport this pr to 0.7.
It's OK for me.
--
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 #616: [#571][FOLLOWUP] don't recreate base dir if it's already an dir
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #616:
URL: https://github.com/apache/incubator-uniffle/pull/616#issuecomment-1432998630
# [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/616?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 [#616](https://codecov.io/gh/apache/incubator-uniffle/pull/616?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0194e0a) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/d0ef8277c487e3d6f9811a924da869a7fb36a75d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d0ef827) will **decrease** coverage by `0.01%`.
> The diff coverage is `33.33%`.
```diff
@@ Coverage Diff @@
## master #616 +/- ##
============================================
- Coverage 60.87% 60.86% -0.01%
Complexity 1802 1802
============================================
Files 214 214
Lines 12388 12391 +3
Branches 1045 1046 +1
============================================
+ Hits 7541 7542 +1
- Misses 4442 4444 +2
Partials 405 405
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/616?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...rg/apache/uniffle/storage/common/LocalStorage.java](https://codecov.io/gh/apache/incubator-uniffle/pull/616?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.76% <33.33%> (-0.30%)` | :arrow_down: |
: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 #616: [#571][FOLLOWUP] fix: don't recreate base dir if it's already an dir
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #616:
URL: https://github.com/apache/incubator-uniffle/pull/616#discussion_r1109280285
##########
storage/src/main/java/org/apache/uniffle/storage/common/LocalStorage.java:
##########
@@ -78,9 +78,15 @@ private LocalStorage(Builder builder) {
File baseFolder = new File(basePath);
try {
- if (isEmptyAndWritableDir(baseFolder)) {
- LOG.warn("Base directory is already an empty dir, skip init");
+ // base folder could be the mounted path, which could not be deleted.
+ // just clean the directory instead.
+ if (baseFolder.isDirectory()) {
+ LOG.info("Base directory is already existed, just clean the directory");
+ FileUtils.cleanDirectory(baseFolder);
} else {
+ // base folder could be not existed or a regular file instead of dir.
+ // just try to deleteDirectory and recreate it. `deleteDirectory` would throw an exception
+ // when the base folder is a file.
FileUtils.deleteDirectory(baseFolder);
Review Comment:
Please see L87.
--
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 merged pull request #616: [#571][FOLLOWUP] fix: optimize base dir init process
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy merged PR #616:
URL: https://github.com/apache/incubator-uniffle/pull/616
--
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 #616: [#571][FOLLOWUP] fix: don't recreate base dir if it's already an dir
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #616:
URL: https://github.com/apache/incubator-uniffle/pull/616#discussion_r1109215635
##########
storage/src/main/java/org/apache/uniffle/storage/common/LocalStorage.java:
##########
@@ -81,9 +81,16 @@ private LocalStorage(Builder builder) {
if (isEmptyAndWritableDir(baseFolder)) {
LOG.warn("Base directory is already an empty dir, skip init");
} else {
- FileUtils.deleteDirectory(baseFolder);
- if (!baseFolder.mkdirs()) {
- throw new IOException("Failed to create base folder: " + basePath);
+ // base folder could be the mounted path, which could not be deleted.
+ // just clean the directory instead.
+ if (baseFolder.isDirectory()) {
+ LOG.warn("Base directory is already existed, just clean");
Review Comment:
warn or info are both ok.
--
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] jerqi commented on pull request #616: [#571][FOLLOWUP] fix: don't recreate base dir if it's already an dir
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #616:
URL: https://github.com/apache/incubator-uniffle/pull/616#issuecomment-1434006790
> > Should this pr be included in version 0.7?
>
> Yes.
If this pr is merged, we should backport this pr to 0.7.
--
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 #616: [#571][FOLLOWUP] fix: don't recreate base dir if it's already an dir
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #616:
URL: https://github.com/apache/incubator-uniffle/pull/616#issuecomment-1434005395
> Should this pr be included in version 0.7?
Yes.
--
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 #616: [#571][FOLLOWUP] don't recreate base dir if it's already an dir
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #616:
URL: https://github.com/apache/incubator-uniffle/pull/616#discussion_r1108684127
##########
storage/src/main/java/org/apache/uniffle/storage/common/LocalStorage.java:
##########
@@ -81,9 +81,16 @@ private LocalStorage(Builder builder) {
if (isEmptyAndWritableDir(baseFolder)) {
LOG.warn("Base directory is already an empty dir, skip init");
} else {
- FileUtils.deleteDirectory(baseFolder);
- if (!baseFolder.mkdirs()) {
- throw new IOException("Failed to create base folder: " + basePath);
+ // base folder could be the mounted path, which could not be deleted.
+ // just clean the directory instead.
+ if (baseFolder.isDirectory()) {
Review Comment:
Concat is to line 83, i.e. `} else if () {`.
Maybe you should check `isDirectory` before `isEmptyAndWritableDir`.
--
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