You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by bowenli86 <gi...@git.apache.org> on 2017/10/11 10:44:18 UTC
[GitHub] flink pull request #4798: [FLINK-6505] Proactively cleanup local FS for Rock...
GitHub user bowenli86 opened a pull request:
https://github.com/apache/flink/pull/4798
[FLINK-6505] Proactively cleanup local FS for RocksDBKeyedStateBackend on startup
## What is the purpose of the change
In `RocksDBKeyedStateBackend`, the `instanceBasePath` is cleared on `dispose()`. It also make sense to also clear this directory when the backend is created, in case something crashed and the backend never reached `dispose()`. At least for previous runs of the same job, we can know what to delete on restart.
In general, it is very important for this backend to clean up the local FS, because the local quota might be very limited compared to the DFS. And a node that runs out of local disk space can bring down the whole job, with no way to recover (it might always get rescheduled to that node).
## Brief change log
clear `instanceBasePath` when `RocksDBKeyedStateBackend ` is created
## 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:
none
## Documentation
none
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bowenli86/flink FLINK-6505
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4798.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 #4798
----
commit 3c4ea092759f2df052cc3dc02403d041f4c16b2d
Author: Bowen Li <bo...@gmail.com>
Date: 2017-10-10T05:31:17Z
[FLIN-6505] Proactively cleanup local FS for RocksDBKeyedStateBackend on startup
commit 22a761736d43114fb5b935d53df65bcf3832f02d
Author: Bowen Li <bo...@gmail.com>
Date: 2017-10-11T10:41:24Z
add comment
----
---
[GitHub] flink issue #4798: [FLINK-6505] Proactively cleanup local FS for RocksDBKeye...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/4798
I only had a minor comment, overall the PR looks good! 👍
---
[GitHub] flink pull request #4798: [FLINK-6505] Proactively cleanup local FS for Rock...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/4798#discussion_r146161007
--- Diff: flink-contrib/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBKeyedStateBackend.java ---
@@ -313,10 +317,16 @@ public void dispose() {
IOUtils.closeQuietly(dbOptions);
IOUtils.closeQuietly(columnOptions);
+ cleanInstanceBasePath();
--- End diff --
This again would not need the existence check that runs inside the method, because at this point the directory should always exist.
---
[GitHub] flink pull request #4798: [FLINK-6505] Proactively cleanup local FS for Rock...
Posted by bowenli86 <gi...@git.apache.org>.
Github user bowenli86 commented on a diff in the pull request:
https://github.com/apache/flink/pull/4798#discussion_r146165645
--- Diff: flink-contrib/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBKeyedStateBackend.java ---
@@ -235,6 +235,10 @@ public RocksDBKeyedStateBackend(
this.instanceBasePath = Preconditions.checkNotNull(instanceBasePath);
this.instanceRocksDBPath = new File(instanceBasePath, "db");
+ // Clear this directory when the backend is created
+ // in case something crashed and the backend never reached dispose()
+ cleanInstanceBasePath();
+
if (!instanceBasePath.exists()) {
--- End diff --
You are right
---
[GitHub] flink issue #4798: [FLINK-6505] Proactively cleanup local FS for RocksDBKeye...
Posted by bowenli86 <gi...@git.apache.org>.
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4798
@StefanRRichter @aljoscha is it still possible to get this into 1.4?
---
[GitHub] flink issue #4798: [FLINK-6505] Proactively cleanup local FS for RocksDBKeye...
Posted by bowenli86 <gi...@git.apache.org>.
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4798
Hi @StephanEwen @StefanRRichter , please let me know if you have any more feedbacks
---
[GitHub] flink pull request #4798: [FLINK-6505] Proactively cleanup local FS for Rock...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/4798#discussion_r146160907
--- Diff: flink-contrib/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBKeyedStateBackend.java ---
@@ -235,6 +235,10 @@ public RocksDBKeyedStateBackend(
this.instanceBasePath = Preconditions.checkNotNull(instanceBasePath);
this.instanceRocksDBPath = new File(instanceBasePath, "db");
+ // Clear this directory when the backend is created
+ // in case something crashed and the backend never reached dispose()
+ cleanInstanceBasePath();
+
if (!instanceBasePath.exists()) {
--- End diff --
nit: you could already integrate the deletion with this existence check, otherwise the check is often executed twice for no good reason.
---
[GitHub] flink pull request #4798: [FLINK-6505] Proactively cleanup local FS for Rock...
Posted by bowenli86 <gi...@git.apache.org>.
Github user bowenli86 closed the pull request at:
https://github.com/apache/flink/pull/4798
---
[GitHub] flink issue #4798: [FLINK-6505] Proactively cleanup local FS for RocksDBKeye...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/4798
+1 to having this in 1.4. @StefanRRichter, can you please merge once you're satisfied?
---
[GitHub] flink issue #4798: [FLINK-6505] Proactively cleanup local FS for RocksDBKeye...
Posted by bowenli86 <gi...@git.apache.org>.
Github user bowenli86 commented on the issue:
https://github.com/apache/flink/pull/4798
Hi @StefanRRichter , do you have more feedbacks?
---
[GitHub] flink pull request #4798: [FLINK-6505] Proactively cleanup local FS for Rock...
Posted by bowenli86 <gi...@git.apache.org>.
Github user bowenli86 commented on a diff in the pull request:
https://github.com/apache/flink/pull/4798#discussion_r146167573
--- Diff: flink-contrib/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBKeyedStateBackend.java ---
@@ -235,26 +235,16 @@ public RocksDBKeyedStateBackend(
this.instanceBasePath = Preconditions.checkNotNull(instanceBasePath);
this.instanceRocksDBPath = new File(instanceBasePath, "db");
- // Clear this directory when the backend is created
+ // Clear the base directory when the backend is created
// in case something crashed and the backend never reached dispose()
- cleanInstanceBasePath();
-
- if (!instanceBasePath.exists()) {
+ if (instanceBasePath.exists()) {
+ cleanInstanceBasePath();
--- End diff --
my bad... I'll break them into two `if` to make sure the `instanceBasePath` will always be created
---
[GitHub] flink issue #4798: [FLINK-6505] Proactively cleanup local FS for RocksDBKeye...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/4798
+1 also from my side, I will merge this.
---
[GitHub] flink issue #4798: [FLINK-6505] Proactively cleanup local FS for RocksDBKeye...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4798
Probably good change for now.
I think in the long run, the TaskManager should give each Task a sub-directory and make sure that sub directory is cleared whenever tasks finish/cancel/fail. That way this safety net is more general.
---
[GitHub] flink pull request #4798: [FLINK-6505] Proactively cleanup local FS for Rock...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/4798#discussion_r146166380
--- Diff: flink-contrib/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBKeyedStateBackend.java ---
@@ -235,26 +235,16 @@ public RocksDBKeyedStateBackend(
this.instanceBasePath = Preconditions.checkNotNull(instanceBasePath);
this.instanceRocksDBPath = new File(instanceBasePath, "db");
- // Clear this directory when the backend is created
+ // Clear the base directory when the backend is created
// in case something crashed and the backend never reached dispose()
- cleanInstanceBasePath();
-
- if (!instanceBasePath.exists()) {
+ if (instanceBasePath.exists()) {
+ cleanInstanceBasePath();
--- End diff --
Unfortunately, this looks incorrect now, because the directory is actually deleted and the `else` branch is not hit. There is also a method to just clear directory content, but then different methods should be used here and in dispose (which can actually delete the directory).
Nit: could merge `else`+ `if` into `else if`.
---
[GitHub] flink pull request #4798: [FLINK-6505] Proactively cleanup local FS for Rock...
Posted by bowenli86 <gi...@git.apache.org>.
Github user bowenli86 commented on a diff in the pull request:
https://github.com/apache/flink/pull/4798#discussion_r146165655
--- Diff: flink-contrib/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBKeyedStateBackend.java ---
@@ -313,10 +317,16 @@ public void dispose() {
IOUtils.closeQuietly(dbOptions);
IOUtils.closeQuietly(columnOptions);
+ cleanInstanceBasePath();
--- End diff --
addressed
---
[GitHub] flink issue #4798: [FLINK-6505] Proactively cleanup local FS for RocksDBKeye...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/4798
Done. @bowenli86 Can you please close this PR?
---