You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by yanghua <gi...@git.apache.org> on 2018/03/28 02:45:44 UTC
[GitHub] flink pull request #5777: [FLINK-7897] Consider using nio.Files for file del...
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
----
----
---
[GitHub] flink pull request #5777: [FLINK-7897] Consider using nio.Files for file del...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on a diff in the pull request:
https://github.com/apache/flink/pull/5777#discussion_r177777396
--- 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.
---
[GitHub] flink issue #5777: [FLINK-7897] Consider using nio.Files for file deletion i...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/5777
LGTM 👍 Will merge.
---
[GitHub] flink pull request #5777: [FLINK-7897] Consider using nio.Files for file del...
Posted by yuqi1129 <gi...@git.apache.org>.
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.
---
[GitHub] flink issue #5777: [FLINK-7897] Consider using nio.Files for file deletion i...
Posted by yuqi1129 <gi...@git.apache.org>.
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
---
[GitHub] flink pull request #5777: [FLINK-7897] Consider using nio.Files for file del...
Posted by StefanRRichter <gi...@git.apache.org>.
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
---
[GitHub] flink issue #5777: [FLINK-7897] Consider using nio.Files for file deletion i...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/5777
cc @zentol
---
[GitHub] flink issue #5777: [FLINK-7897] Consider using nio.Files for file deletion i...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/5777
cc @tillrohrmann
---
[GitHub] flink issue #5777: [FLINK-7897] Consider using nio.Files for file deletion i...
Posted by yanghua <gi...@git.apache.org>.
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.
---
[GitHub] flink issue #5777: [FLINK-7897] Consider using nio.Files for file deletion i...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/5777
cc @zentol
---
[GitHub] flink pull request #5777: [FLINK-7897] Consider using nio.Files for file del...
Posted by StefanRRichter <gi...@git.apache.org>.
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?
---
[GitHub] flink pull request #5777: [FLINK-7897] Consider using nio.Files for file del...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/5777
---
[GitHub] flink issue #5777: [FLINK-7897] Consider using nio.Files for file deletion i...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/5777
@StefanRRichter , does this PR look good to you?
---
[GitHub] flink issue #5777: [FLINK-7897] Consider using nio.Files for file deletion i...
Posted by yanghua <gi...@git.apache.org>.
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?
---
[GitHub] flink pull request #5777: [FLINK-7897] Consider using nio.Files for file del...
Posted by StefanRRichter <gi...@git.apache.org>.
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
---
[GitHub] flink issue #5777: [FLINK-7897] Consider using nio.Files for file deletion i...
Posted by tedyu <gi...@git.apache.org>.
Github user tedyu commented on the issue:
https://github.com/apache/flink/pull/5777
@yuqi1129
What do you think ?
---