[jira] [Commented] (FLINK-7897) Consider using nio.Files for file deletion in TransientBlobCleanupTask
[ https://issues.apache.org/jira/browse/FLINK-7897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16523388#comment-16523388 ] ASF GitHub Bot commented on FLINK-7897: --- Github user StefanRRichter commented on the issue: https://github.com/apache/flink/pull/5777 LGTM Will merge. > Consider using nio.Files for file deletion in TransientBlobCleanupTask > -- > > Key: FLINK-7897 > URL: https://issues.apache.org/jira/browse/FLINK-7897 > Project: Flink > Issue Type: Improvement > Components: Local Runtime >Reporter: Ted Yu >Assignee: vinoyang >Priority: Minor > Labels: pull-request-available > > nio.Files#delete() provides better clue as to why the deletion may fail: > https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#delete(java.nio.file.Path) > Depending on the potential exception (FileNotFound), the call to > localFile.exists() may be skipped. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-7897) Consider using nio.Files for file deletion in TransientBlobCleanupTask
[ https://issues.apache.org/jira/browse/FLINK-7897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16523396#comment-16523396 ] ASF GitHub Bot commented on FLINK-7897: --- Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/5777 > Consider using nio.Files for file deletion in TransientBlobCleanupTask > -- > > Key: FLINK-7897 > URL: https://issues.apache.org/jira/browse/FLINK-7897 > Project: Flink > Issue Type: Improvement > Components: Local Runtime >Reporter: Ted Yu >Assignee: vinoyang >Priority: Minor > Labels: pull-request-available > > nio.Files#delete() provides better clue as to why the deletion may fail: > https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#delete(java.nio.file.Path) > Depending on the potential exception (FileNotFound), the call to > localFile.exists() may be skipped. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-7897) Consider using nio.Files for file deletion in TransientBlobCleanupTask
[ https://issues.apache.org/jira/browse/FLINK-7897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16517233#comment-16517233 ] ASF GitHub Bot commented on FLINK-7897: --- Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5777 @StefanRRichter , does this PR look good to you? > Consider using nio.Files for file deletion in TransientBlobCleanupTask > -- > > Key: FLINK-7897 > URL: https://issues.apache.org/jira/browse/FLINK-7897 > Project: Flink > Issue Type: Improvement > Components: Local Runtime >Reporter: Ted Yu >Assignee: vinoyang >Priority: Minor > > nio.Files#delete() provides better clue as to why the deletion may fail: > https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#delete(java.nio.file.Path) > Depending on the potential exception (FileNotFound), the call to > localFile.exists() may be skipped. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-7897) Consider using nio.Files for file deletion in TransientBlobCleanupTask
[
https://issues.apache.org/jira/browse/FLINK-7897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16512268#comment-16512268
]
ASF GitHub Bot commented on FLINK-7897:
---
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/5777#discussion_r195370225
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/blob/TransientBlobCleanupTask.java
---
@@ -100,9 +102,15 @@ public void run() {
writeLock.lock();
try {
- if (!localFile.delete() &&
localFile.exists()) {
- log.warn("Failed to locally
delete blob " + localFile.getAbsolutePath());
- } else {
+ try {
+
Files.delete(localFile.toPath());
+ } catch (IOException e) {
+ log.error("Failed to delete
locally blob " + localFile.getAbsolutePath(), e);
+ } catch (Exception e) {
+ log.error("Failed to delete
locally blob " + localFile.getAbsolutePath(), e);
+ }
+
+ if (!localFile.exists()) {
--- End diff --
Is there any cause that this check is required here? Why not just remove
the entry after `Files.delete(...)` at the end of the `try` block
> Consider using nio.Files for file deletion in TransientBlobCleanupTask
> --
>
> Key: FLINK-7897
> URL: https://issues.apache.org/jira/browse/FLINK-7897
> Project: Flink
> Issue Type: Improvement
> Components: Local Runtime
>Reporter: Ted Yu
>Assignee: vinoyang
>Priority: Minor
>
> nio.Files#delete() provides better clue as to why the deletion may fail:
> https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#delete(java.nio.file.Path)
> Depending on the potential exception (FileNotFound), the call to
> localFile.exists() may be skipped.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (FLINK-7897) Consider using nio.Files for file deletion in TransientBlobCleanupTask
[
https://issues.apache.org/jira/browse/FLINK-7897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16512264#comment-16512264
]
ASF GitHub Bot commented on FLINK-7897:
---
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/5777#discussion_r195369853
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/blob/TransientBlobCleanupTask.java
---
@@ -100,9 +102,15 @@ public void run() {
writeLock.lock();
try {
- if (!localFile.delete() &&
localFile.exists()) {
- log.warn("Failed to locally
delete blob " + localFile.getAbsolutePath());
- } else {
+ try {
+
Files.delete(localFile.toPath());
+ } catch (IOException e) {
+ log.error("Failed to delete
locally blob " + localFile.getAbsolutePath(), e);
+ } catch (Exception e) {
+ log.error("Failed to delete
locally blob " + localFile.getAbsolutePath(), e);
--- End diff --
typo: locally -> local
> Consider using nio.Files for file deletion in TransientBlobCleanupTask
> --
>
> Key: FLINK-7897
> URL: https://issues.apache.org/jira/browse/FLINK-7897
> Project: Flink
> Issue Type: Improvement
> Components: Local Runtime
>Reporter: Ted Yu
>Assignee: vinoyang
>Priority: Minor
>
> nio.Files#delete() provides better clue as to why the deletion may fail:
> https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#delete(java.nio.file.Path)
> Depending on the potential exception (FileNotFound), the call to
> localFile.exists() may be skipped.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (FLINK-7897) Consider using nio.Files for file deletion in TransientBlobCleanupTask
[
https://issues.apache.org/jira/browse/FLINK-7897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16512263#comment-16512263
]
ASF GitHub Bot commented on FLINK-7897:
---
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/5777#discussion_r195369715
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/blob/TransientBlobCleanupTask.java
---
@@ -100,9 +102,15 @@ public void run() {
writeLock.lock();
try {
- if (!localFile.delete() &&
localFile.exists()) {
- log.warn("Failed to locally
delete blob " + localFile.getAbsolutePath());
- } else {
+ try {
+
Files.delete(localFile.toPath());
+ } catch (IOException e) {
--- End diff --
Why are `IOException` and `Exception` caught separately if the action is
the same?
> Consider using nio.Files for file deletion in TransientBlobCleanupTask
> --
>
> Key: FLINK-7897
> URL: https://issues.apache.org/jira/browse/FLINK-7897
> Project: Flink
> Issue Type: Improvement
> Components: Local Runtime
>Reporter: Ted Yu
>Assignee: vinoyang
>Priority: Minor
>
> nio.Files#delete() provides better clue as to why the deletion may fail:
> https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#delete(java.nio.file.Path)
> Depending on the potential exception (FileNotFound), the call to
> localFile.exists() may be skipped.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (FLINK-7897) Consider using nio.Files for file deletion in TransientBlobCleanupTask
[ https://issues.apache.org/jira/browse/FLINK-7897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16510795#comment-16510795 ] ASF GitHub Bot commented on FLINK-7897: --- Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5777 cc @zentol this PR takes a long time, can you review this? > Consider using nio.Files for file deletion in TransientBlobCleanupTask > -- > > Key: FLINK-7897 > URL: https://issues.apache.org/jira/browse/FLINK-7897 > Project: Flink > Issue Type: Improvement > Components: Local Runtime >Reporter: Ted Yu >Assignee: vinoyang >Priority: Minor > > nio.Files#delete() provides better clue as to why the deletion may fail: > https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#delete(java.nio.file.Path) > Depending on the potential exception (FileNotFound), the call to > localFile.exists() may be skipped. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-7897) Consider using nio.Files for file deletion in TransientBlobCleanupTask
[ https://issues.apache.org/jira/browse/FLINK-7897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16466011#comment-16466011 ] ASF GitHub Bot commented on FLINK-7897: --- Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5777 cc @zentol @GJL @tzulitai this PR takes a long time, please review it thanks. > Consider using nio.Files for file deletion in TransientBlobCleanupTask > -- > > Key: FLINK-7897 > URL: https://issues.apache.org/jira/browse/FLINK-7897 > Project: Flink > Issue Type: Improvement > Components: Local Runtime >Reporter: Ted Yu >Assignee: vinoyang >Priority: Minor > > nio.Files#delete() provides better clue as to why the deletion may fail: > https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#delete(java.nio.file.Path) > Depending on the potential exception (FileNotFound), the call to > localFile.exists() may be skipped. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-7897) Consider using nio.Files for file deletion in TransientBlobCleanupTask
[ https://issues.apache.org/jira/browse/FLINK-7897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16452160#comment-16452160 ] ASF GitHub Bot commented on FLINK-7897: --- Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5777 cc @zentol > Consider using nio.Files for file deletion in TransientBlobCleanupTask > -- > > Key: FLINK-7897 > URL: https://issues.apache.org/jira/browse/FLINK-7897 > Project: Flink > Issue Type: Improvement > Components: Local Runtime >Reporter: Ted Yu >Assignee: vinoyang >Priority: Minor > > nio.Files#delete() provides better clue as to why the deletion may fail: > https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#delete(java.nio.file.Path) > Depending on the potential exception (FileNotFound), the call to > localFile.exists() may be skipped. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-7897) Consider using nio.Files for file deletion in TransientBlobCleanupTask
[ https://issues.apache.org/jira/browse/FLINK-7897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16443722#comment-16443722 ] ASF GitHub Bot commented on FLINK-7897: --- Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5777 cc @zentol > Consider using nio.Files for file deletion in TransientBlobCleanupTask > -- > > Key: FLINK-7897 > URL: https://issues.apache.org/jira/browse/FLINK-7897 > Project: Flink > Issue Type: Improvement > Components: Local Runtime >Reporter: Ted Yu >Assignee: vinoyang >Priority: Minor > > nio.Files#delete() provides better clue as to why the deletion may fail: > https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#delete(java.nio.file.Path) > Depending on the potential exception (FileNotFound), the call to > localFile.exists() may be skipped. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-7897) Consider using nio.Files for file deletion in TransientBlobCleanupTask
[ https://issues.apache.org/jira/browse/FLINK-7897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16441585#comment-16441585 ] Ted Yu commented on FLINK-7897: --- lgtm > Consider using nio.Files for file deletion in TransientBlobCleanupTask > -- > > Key: FLINK-7897 > URL: https://issues.apache.org/jira/browse/FLINK-7897 > Project: Flink > Issue Type: Improvement > Components: Local Runtime >Reporter: Ted Yu >Assignee: vinoyang >Priority: Minor > > nio.Files#delete() provides better clue as to why the deletion may fail: > https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#delete(java.nio.file.Path) > Depending on the potential exception (FileNotFound), the call to > localFile.exists() may be skipped. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-7897) Consider using nio.Files for file deletion in TransientBlobCleanupTask
[ https://issues.apache.org/jira/browse/FLINK-7897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16431632#comment-16431632 ] ASF GitHub Bot commented on FLINK-7897: --- Github user yuqi1129 commented on the issue: https://github.com/apache/flink/pull/5777 @tedyu , You are right, I ignore the code below which do not show in the change. +1 > Consider using nio.Files for file deletion in TransientBlobCleanupTask > -- > > Key: FLINK-7897 > URL: https://issues.apache.org/jira/browse/FLINK-7897 > Project: Flink > Issue Type: Improvement > Components: Local Runtime >Reporter: Ted Yu >Assignee: vinoyang >Priority: Minor > > nio.Files#delete() provides better clue as to why the deletion may fail: > https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#delete(java.nio.file.Path) > Depending on the potential exception (FileNotFound), the call to > localFile.exists() may be skipped. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-7897) Consider using nio.Files for file deletion in TransientBlobCleanupTask
[ https://issues.apache.org/jira/browse/FLINK-7897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16431281#comment-16431281 ] ASF GitHub Bot commented on FLINK-7897: --- Github user tedyu commented on the issue: https://github.com/apache/flink/pull/5777 @yuqi1129 What do you think ? > Consider using nio.Files for file deletion in TransientBlobCleanupTask > -- > > Key: FLINK-7897 > URL: https://issues.apache.org/jira/browse/FLINK-7897 > Project: Flink > Issue Type: Improvement > Components: Local Runtime >Reporter: Ted Yu >Assignee: vinoyang >Priority: Minor > > nio.Files#delete() provides better clue as to why the deletion may fail: > https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#delete(java.nio.file.Path) > Depending on the potential exception (FileNotFound), the call to > localFile.exists() may be skipped. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-7897) Consider using nio.Files for file deletion in TransientBlobCleanupTask
[
https://issues.apache.org/jira/browse/FLINK-7897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16417461#comment-16417461
]
ASF GitHub Bot commented on FLINK-7897:
---
Github user yanghua commented on a diff in the pull request:
https://github.com/apache/flink/pull/5777#discussion_r17396
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/blob/TransientBlobCleanupTask.java
---
@@ -100,9 +102,15 @@ public void run() {
writeLock.lock();
try {
- if (!localFile.delete() &&
localFile.exists()) {
- log.warn("Failed to locally
delete blob " + localFile.getAbsolutePath());
- } else {
+ try {
--- End diff --
@yuqi1129 I think there is no wrong about the changes. Considering the
original code, if one of the two conditions : `localFile.delete()` returns
`true` or _file does not exist_ matched. The code `entries.remove(entry);` will
be invoked. That means if there is no file (no mater it has been deleted or
really not exists) , the entries can only be removed.
My changes make sure that if there is no file, the entries will be removed.
if the file exists it will retained. The `Files.delete` in nio package just
throw exception but not return bool value. And the original `File#delete` can
return bool value and throw exception. You can read specific javadoc.
> Consider using nio.Files for file deletion in TransientBlobCleanupTask
> --
>
> Key: FLINK-7897
> URL: https://issues.apache.org/jira/browse/FLINK-7897
> Project: Flink
> Issue Type: Improvement
> Components: Local Runtime
>Reporter: Ted Yu
>Assignee: vinoyang
>Priority: Minor
>
> nio.Files#delete() provides better clue as to why the deletion may fail:
> https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#delete(java.nio.file.Path)
> Depending on the potential exception (FileNotFound), the call to
> localFile.exists() may be skipped.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (FLINK-7897) Consider using nio.Files for file deletion in TransientBlobCleanupTask
[
https://issues.apache.org/jira/browse/FLINK-7897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16417286#comment-16417286
]
ASF GitHub Bot commented on FLINK-7897:
---
Github user yuqi1129 commented on a diff in the pull request:
https://github.com/apache/flink/pull/5777#discussion_r177736130
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/blob/TransientBlobCleanupTask.java
---
@@ -100,9 +102,15 @@ public void run() {
writeLock.lock();
try {
- if (!localFile.delete() &&
localFile.exists()) {
- log.warn("Failed to locally
delete blob " + localFile.getAbsolutePath());
- } else {
+ try {
--- End diff --
According to code before, entries will be deleted if no exception occur
whether file was deleted or not. As for you code logic, if We failed to delete
file, entries will stay in the Map.
> Consider using nio.Files for file deletion in TransientBlobCleanupTask
> --
>
> Key: FLINK-7897
> URL: https://issues.apache.org/jira/browse/FLINK-7897
> Project: Flink
> Issue Type: Improvement
> Components: Local Runtime
>Reporter: Ted Yu
>Assignee: vinoyang
>Priority: Minor
>
> nio.Files#delete() provides better clue as to why the deletion may fail:
> https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#delete(java.nio.file.Path)
> Depending on the potential exception (FileNotFound), the call to
> localFile.exists() may be skipped.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (FLINK-7897) Consider using nio.Files for file deletion in TransientBlobCleanupTask
[ https://issues.apache.org/jira/browse/FLINK-7897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16416807#comment-16416807 ] ASF GitHub Bot commented on FLINK-7897: --- Github user yanghua commented on the issue: https://github.com/apache/flink/pull/5777 cc @tillrohrmann > Consider using nio.Files for file deletion in TransientBlobCleanupTask > -- > > Key: FLINK-7897 > URL: https://issues.apache.org/jira/browse/FLINK-7897 > Project: Flink > Issue Type: Improvement > Components: Local Runtime >Reporter: Ted Yu >Assignee: vinoyang >Priority: Minor > > nio.Files#delete() provides better clue as to why the deletion may fail: > https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#delete(java.nio.file.Path) > Depending on the potential exception (FileNotFound), the call to > localFile.exists() may be skipped. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (FLINK-7897) Consider using nio.Files for file deletion in TransientBlobCleanupTask
[ https://issues.apache.org/jira/browse/FLINK-7897?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16416625#comment-16416625 ] ASF GitHub Bot commented on FLINK-7897: --- GitHub user yanghua opened a pull request: https://github.com/apache/flink/pull/5777 [FLINK-7897] Consider using nio.Files for file deletion in TransientBlobCleanupTask ## What is the purpose of the change *This pull request use `nio.Files.delete` method to make the delete failed reason more clear* ## Brief change log - *use nio.Files.delete static method to replace file#delete method* ## Verifying this change This change is a trivial rework / code cleanup without any test coverage. ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): (yes / **no**) - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes / **no**) - The serializers: (yes / **no** / don't know) - The runtime per-record code paths (performance sensitive): (yes / **no** / don't know) - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / **no** / don't know) - The S3 file system connector: (yes / **no** / don't know) ## Documentation - Does this pull request introduce a new feature? (yes / **no**) - If yes, how is the feature documented? (not applicable / docs / JavaDocs / **not documented**) You can merge this pull request into a Git repository by running: $ git pull https://github.com/yanghua/flink FLINK-7897 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/5777.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #5777 > Consider using nio.Files for file deletion in TransientBlobCleanupTask > -- > > Key: FLINK-7897 > URL: https://issues.apache.org/jira/browse/FLINK-7897 > Project: Flink > Issue Type: Improvement > Components: Local Runtime >Reporter: Ted Yu >Assignee: vinoyang >Priority: Minor > > nio.Files#delete() provides better clue as to why the deletion may fail: > https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#delete(java.nio.file.Path) > Depending on the potential exception (FileNotFound), the call to > localFile.exists() may be skipped. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
