You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2020/02/14 12:47:40 UTC
[GitHub] [flink] StephanEwen opened a new pull request #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
StephanEwen opened a new pull request #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095
## What is the purpose of the change
This solves the bug that Paths are handled wrong on Windows, breaking incremental checkpoints.
The bug manifests in errors like
```
org.rocksdb.RocksDBException: Unexpected error for: /C:/Users/ewens/AppData/Local/Temp/... ...-77c716fd2ade/chk-682375462378: The filename, directory name, or volume label syntax is incorrect.
```
The bug was caused by wrong path handling logic on Windows. RocksDB's initial native checkpoint (flush/hardlink) needs to go lo the local filesystem anyways, so we can directly use local path class for that, which does not suffer from cross-platform escaping issues.
## Brief change log
- Replace use of `org.apache.flink.core.fs.Path` with `java.nio.file.Path` in RocksDB incremental checkpoint classes.
## Verifying this change
This change is already covered by the tests for RocksDB checkpointing.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): **no**
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: **no**
- The serializers: **no**
- The runtime per-record code paths (performance sensitive): **no**
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: **yes**
- The S3 file system connector: **no**
## Documentation
- Does this pull request introduce a new feature? **no**
- If yes, how is the feature documented? **not applicable**
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] StephanEwen commented on issue #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
StephanEwen commented on issue #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#issuecomment-586287328
Urg, that a nasty "gotcha". Will move this as a helper into `FileUtils`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on a change in pull request #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#discussion_r379417283
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/state/SnapshotDirectory.java
##########
@@ -49,50 +49,37 @@
@Nonnull
protected final Path directory;
- /** The filesystem that contains the snapshot directory. */
- @Nonnull
- protected final FileSystem fileSystem;
-
/** This reference tracks the lifecycle state of the snapshot directory. */
@Nonnull
protected AtomicReference<State> state;
- private SnapshotDirectory(@Nonnull Path directory, @Nonnull FileSystem fileSystem) {
+ private SnapshotDirectory(@Nonnull Path directory) {
this.directory = directory;
- this.fileSystem = fileSystem;
this.state = new AtomicReference<>(State.ONGOING);
}
- private SnapshotDirectory(@Nonnull Path directory) throws IOException {
- this(directory, directory.getFileSystem());
- }
-
@Nonnull
public Path getDirectory() {
return directory;
}
public boolean mkdirs() throws IOException {
- return fileSystem.mkdirs(directory);
- }
-
- @Nonnull
- public FileSystem getFileSystem() {
- return fileSystem;
+ Files.createDirectories(directory);
+ return true;
}
public boolean exists() throws IOException {
- return fileSystem.exists(directory);
+ return Files.exists(directory);
}
/**
- * List the statuses of the files/directories in the snapshot directory.
+ * List the files in the snapshot directory.
*
- * @return the statuses of the files/directories in the given path.
+ * @return the files in the snapshot directory.
* @throws IOException if there is a problem creating the file statuses.
*/
- public FileStatus[] listStatus() throws IOException {
- return fileSystem.listStatus(directory);
+ public Path[] listDirectory() throws IOException {
+ return Files.list(directory).toArray(Path[]::new);
Review comment:
IIRC the `Stream<Path>` returned by `Files.list` must be explicitly closed, see FLINK-10690.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#issuecomment-586287248
<!--
Meta data
Hash:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/148979015 TriggerType:PUSH TriggerID:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7
Hash:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=5179 TriggerType:PUSH TriggerID:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7
Hash:0412a4320a08135f4f6c1f580e6eb60b7f763551 Status:UNKNOWN URL:TBD TriggerType:PUSH TriggerID:0412a4320a08135f4f6c1f580e6eb60b7f763551
-->
## CI report:
* 1529c3a8abb5e2b758e6a80e2f7359288f87e3d7 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/148979015) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=5179)
* 0412a4320a08135f4f6c1f580e6eb60b7f763551 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] klion26 commented on a change in pull request #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
klion26 commented on a change in pull request #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#discussion_r379820869
##########
File path: flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBStateDownloader.java
##########
@@ -120,14 +121,14 @@ private void downloadDataForStateHandle(
CloseableRegistry closeableRegistry) throws IOException {
FSDataInputStream inputStream = null;
- FSDataOutputStream outputStream = null;
+ OutputStream outputStream = null;
try {
- FileSystem restoreFileSystem = restoreFilePath.getFileSystem();
inputStream = remoteFileHandle.openInputStream();
closeableRegistry.registerCloseable(inputStream);
- outputStream = restoreFileSystem.create(restoreFilePath, FileSystem.WriteMode.OVERWRITE);
+ Files.createDirectories(restoreFilePath.getParent());
+ outputStream = Files.newOutputStream(restoreFilePath, StandardOpenOption.CREATE, StandardOpenOption.WRITE);
Review comment:
Do we need to add an Option `TRUNCATE_EXISTING` here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot commented on issue #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
flinkbot commented on issue #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#issuecomment-586287248
<!--
Meta data
Hash:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7 Status:UNKNOWN URL:TBD TriggerType:PUSH TriggerID:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7
-->
## CI report:
* 1529c3a8abb5e2b758e6a80e2f7359288f87e3d7 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on a change in pull request #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#discussion_r379417692
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/state/SnapshotDirectoryTest.java
##########
@@ -106,14 +104,13 @@ public void listStatus() throws Exception {
File file = new File(folderA, "test.txt");
Assert.assertTrue(file.createNewFile());
- Path path = new Path(folderA.toURI());
- FileSystem fileSystem = path.getFileSystem();
+ Path path = folderA.toPath();
SnapshotDirectory snapshotDirectory = SnapshotDirectory.permanent(path);
Assert.assertTrue(snapshotDirectory.exists());
Assert.assertEquals(
- Arrays.toString(fileSystem.listStatus(path)),
- Arrays.toString(snapshotDirectory.listStatus()));
+ Arrays.toString(Files.list(path).toArray(Path[]::new)),
Review comment:
same as above
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#issuecomment-586287248
<!--
Meta data
Hash:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/148979015 TriggerType:PUSH TriggerID:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7
Hash:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=5179 TriggerType:PUSH TriggerID:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7
Hash:0412a4320a08135f4f6c1f580e6eb60b7f763551 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=5190 TriggerType:PUSH TriggerID:0412a4320a08135f4f6c1f580e6eb60b7f763551
Hash:0412a4320a08135f4f6c1f580e6eb60b7f763551 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/149011380 TriggerType:PUSH TriggerID:0412a4320a08135f4f6c1f580e6eb60b7f763551
-->
## CI report:
* 1529c3a8abb5e2b758e6a80e2f7359288f87e3d7 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/148979015) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=5179)
* 0412a4320a08135f4f6c1f580e6eb60b7f763551 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/149011380) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=5190)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] StephanEwen commented on a change in pull request #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
StephanEwen commented on a change in pull request #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#discussion_r380308235
##########
File path: flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBStateDownloader.java
##########
@@ -120,14 +121,14 @@ private void downloadDataForStateHandle(
CloseableRegistry closeableRegistry) throws IOException {
FSDataInputStream inputStream = null;
- FSDataOutputStream outputStream = null;
+ OutputStream outputStream = null;
try {
- FileSystem restoreFileSystem = restoreFilePath.getFileSystem();
inputStream = remoteFileHandle.openInputStream();
closeableRegistry.registerCloseable(inputStream);
- outputStream = restoreFileSystem.create(restoreFilePath, FileSystem.WriteMode.OVERWRITE);
+ Files.createDirectories(restoreFilePath.getParent());
+ outputStream = Files.newOutputStream(restoreFilePath, StandardOpenOption.CREATE, StandardOpenOption.WRITE);
Review comment:
I think you are right. I will follow Yu's suggestion below to not supply any option, which implies a `TRUNCATE_EXISTING` option.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on a change in pull request #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#discussion_r379417283
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/state/SnapshotDirectory.java
##########
@@ -49,50 +49,37 @@
@Nonnull
protected final Path directory;
- /** The filesystem that contains the snapshot directory. */
- @Nonnull
- protected final FileSystem fileSystem;
-
/** This reference tracks the lifecycle state of the snapshot directory. */
@Nonnull
protected AtomicReference<State> state;
- private SnapshotDirectory(@Nonnull Path directory, @Nonnull FileSystem fileSystem) {
+ private SnapshotDirectory(@Nonnull Path directory) {
this.directory = directory;
- this.fileSystem = fileSystem;
this.state = new AtomicReference<>(State.ONGOING);
}
- private SnapshotDirectory(@Nonnull Path directory) throws IOException {
- this(directory, directory.getFileSystem());
- }
-
@Nonnull
public Path getDirectory() {
return directory;
}
public boolean mkdirs() throws IOException {
- return fileSystem.mkdirs(directory);
- }
-
- @Nonnull
- public FileSystem getFileSystem() {
- return fileSystem;
+ Files.createDirectories(directory);
+ return true;
}
public boolean exists() throws IOException {
- return fileSystem.exists(directory);
+ return Files.exists(directory);
}
/**
- * List the statuses of the files/directories in the snapshot directory.
+ * List the files in the snapshot directory.
*
- * @return the statuses of the files/directories in the given path.
+ * @return the files in the snapshot directory.
* @throws IOException if there is a problem creating the file statuses.
*/
- public FileStatus[] listStatus() throws IOException {
- return fileSystem.listStatus(directory);
+ public Path[] listDirectory() throws IOException {
+ return Files.list(directory).toArray(Path[]::new);
Review comment:
IIRC `Files.list` must be explicitly closed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on a change in pull request #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#discussion_r379417283
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/state/SnapshotDirectory.java
##########
@@ -49,50 +49,37 @@
@Nonnull
protected final Path directory;
- /** The filesystem that contains the snapshot directory. */
- @Nonnull
- protected final FileSystem fileSystem;
-
/** This reference tracks the lifecycle state of the snapshot directory. */
@Nonnull
protected AtomicReference<State> state;
- private SnapshotDirectory(@Nonnull Path directory, @Nonnull FileSystem fileSystem) {
+ private SnapshotDirectory(@Nonnull Path directory) {
this.directory = directory;
- this.fileSystem = fileSystem;
this.state = new AtomicReference<>(State.ONGOING);
}
- private SnapshotDirectory(@Nonnull Path directory) throws IOException {
- this(directory, directory.getFileSystem());
- }
-
@Nonnull
public Path getDirectory() {
return directory;
}
public boolean mkdirs() throws IOException {
- return fileSystem.mkdirs(directory);
- }
-
- @Nonnull
- public FileSystem getFileSystem() {
- return fileSystem;
+ Files.createDirectories(directory);
+ return true;
}
public boolean exists() throws IOException {
- return fileSystem.exists(directory);
+ return Files.exists(directory);
}
/**
- * List the statuses of the files/directories in the snapshot directory.
+ * List the files in the snapshot directory.
*
- * @return the statuses of the files/directories in the given path.
+ * @return the files in the snapshot directory.
* @throws IOException if there is a problem creating the file statuses.
*/
- public FileStatus[] listStatus() throws IOException {
- return fileSystem.listStatus(directory);
+ public Path[] listDirectory() throws IOException {
+ return Files.list(directory).toArray(Path[]::new);
Review comment:
IIRC `Files.list` most be explicitly closed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on a change in pull request #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#discussion_r379418934
##########
File path: flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/restore/RocksDBIncrementalRestoreOperation.java
##########
@@ -461,26 +457,18 @@ private RestoredDBInstance restoreDBInstanceFromStateHandle(
* a local state.
*/
private void restoreInstanceDirectoryFromPath(Path source, String instanceRocksDBPath) throws IOException {
+ final Path instanceRocksDBDirectory = Paths.get(instanceRocksDBPath);
+ final Path[] files = Files.list(source).toArray(Path[]::new);
Review comment:
same as above
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] carp84 commented on issue #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
carp84 commented on issue #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#issuecomment-590666434
Cherry-picked to release-1.10 and made sure the [travis build](https://travis-ci.org/carp84/flink/builds/654698276) could pass. Pushing the commit to release-1.10 now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] StephanEwen edited a comment on issue #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
StephanEwen edited a comment on issue #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#issuecomment-586354299
Interestingly, with proper eager closing of the directory streams, the workaround in `StateSnapshotTransformerTest` is also no longer necessary.
Apparently the non-closed directory streams kept a handle to the directory, and under the messed up Windows FS semantics, that means the directory cannot be removed / overwritten (unlike posix, where the directory can still be unlinked from the parent).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] StephanEwen commented on a change in pull request #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
StephanEwen commented on a change in pull request #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#discussion_r380308275
##########
File path: flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBStateDownloader.java
##########
@@ -120,14 +121,14 @@ private void downloadDataForStateHandle(
CloseableRegistry closeableRegistry) throws IOException {
FSDataInputStream inputStream = null;
- FSDataOutputStream outputStream = null;
+ OutputStream outputStream = null;
try {
- FileSystem restoreFileSystem = restoreFilePath.getFileSystem();
inputStream = remoteFileHandle.openInputStream();
closeableRegistry.registerCloseable(inputStream);
- outputStream = restoreFileSystem.create(restoreFilePath, FileSystem.WriteMode.OVERWRITE);
+ Files.createDirectories(restoreFilePath.getParent());
+ outputStream = Files.newOutputStream(restoreFilePath, StandardOpenOption.CREATE, StandardOpenOption.WRITE);
Review comment:
Sounds good, will do!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot commented on issue #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
flinkbot commented on issue #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#issuecomment-586274719
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.
## Automated Checks
Last check on commit 1529c3a8abb5e2b758e6a80e2f7359288f87e3d7 (Fri Feb 14 12:50:32 UTC 2020)
**Warnings:**
* No documentation files were touched! Remember to keep the Flink docs up to date!
* **This pull request references an unassigned [Jira ticket](https://issues.apache.org/jira/browse/FLINK-10918).** According to the [code contribution guide](https://flink.apache.org/contributing/contribute-code.html), tickets need to be assigned before starting with the implementation work.
<sub>Mention the bot in a comment to re-run the automated checks.</sub>
## Review Progress
* ❓ 1. The [description] looks good.
* ❓ 2. There is [consensus] that the contribution should go into to Flink.
* ❓ 3. Needs [attention] from.
* ❓ 4. The change fits into the overall [architecture].
* ❓ 5. Overall code [quality] is good.
Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process.<details>
The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required <summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`)
- `@flinkbot approve all` to approve all aspects
- `@flinkbot approve-until architecture` to approve everything until `architecture`
- `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention
- `@flinkbot disapprove architecture` to remove an approval you gave earlier
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] StephanEwen commented on issue #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
StephanEwen commented on issue #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#issuecomment-586405488
@carp84 @Myasuka @klion26 Let me know if you are interested in reviewing this. Otherwise I would merger this in the next days.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on a change in pull request #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#discussion_r379417283
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/state/SnapshotDirectory.java
##########
@@ -49,50 +49,37 @@
@Nonnull
protected final Path directory;
- /** The filesystem that contains the snapshot directory. */
- @Nonnull
- protected final FileSystem fileSystem;
-
/** This reference tracks the lifecycle state of the snapshot directory. */
@Nonnull
protected AtomicReference<State> state;
- private SnapshotDirectory(@Nonnull Path directory, @Nonnull FileSystem fileSystem) {
+ private SnapshotDirectory(@Nonnull Path directory) {
this.directory = directory;
- this.fileSystem = fileSystem;
this.state = new AtomicReference<>(State.ONGOING);
}
- private SnapshotDirectory(@Nonnull Path directory) throws IOException {
- this(directory, directory.getFileSystem());
- }
-
@Nonnull
public Path getDirectory() {
return directory;
}
public boolean mkdirs() throws IOException {
- return fileSystem.mkdirs(directory);
- }
-
- @Nonnull
- public FileSystem getFileSystem() {
- return fileSystem;
+ Files.createDirectories(directory);
+ return true;
}
public boolean exists() throws IOException {
- return fileSystem.exists(directory);
+ return Files.exists(directory);
}
/**
- * List the statuses of the files/directories in the snapshot directory.
+ * List the files in the snapshot directory.
*
- * @return the statuses of the files/directories in the given path.
+ * @return the files in the snapshot directory.
* @throws IOException if there is a problem creating the file statuses.
*/
- public FileStatus[] listStatus() throws IOException {
- return fileSystem.listStatus(directory);
+ public Path[] listDirectory() throws IOException {
+ return Files.list(directory).toArray(Path[]::new);
Review comment:
IIRC the `Stream<Path>` returned by `Files.list` must be explicitly closed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] carp84 commented on a change in pull request #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
carp84 commented on a change in pull request #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#discussion_r379828410
##########
File path: flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBStateDownloader.java
##########
@@ -120,14 +121,14 @@ private void downloadDataForStateHandle(
CloseableRegistry closeableRegistry) throws IOException {
FSDataInputStream inputStream = null;
- FSDataOutputStream outputStream = null;
+ OutputStream outputStream = null;
try {
- FileSystem restoreFileSystem = restoreFilePath.getFileSystem();
inputStream = remoteFileHandle.openInputStream();
closeableRegistry.registerCloseable(inputStream);
- outputStream = restoreFileSystem.create(restoreFilePath, FileSystem.WriteMode.OVERWRITE);
+ Files.createDirectories(restoreFilePath.getParent());
+ outputStream = Files.newOutputStream(restoreFilePath, StandardOpenOption.CREATE, StandardOpenOption.WRITE);
Review comment:
It seems to me we could simply call `Files.newOutputStream(restoreFilePath)` here, which perfectly matches the "overwrite" semantic.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] StephanEwen commented on issue #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
StephanEwen commented on issue #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#issuecomment-587100971
Thanks you for the reviews.
Merging this...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] StephanEwen closed pull request #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
StephanEwen closed pull request #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] StephanEwen edited a comment on issue #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
StephanEwen edited a comment on issue #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#issuecomment-586354299
Interestingly, with proper eager closing of the directory streams, this workaround is also no longer necessary: https://github.com/apache/flink/pull/11095/files#diff-718b96155a7544f30a64f3f389faea3fR67
Apparently the non-closed directory streams kept a handle to the directory, and under the messed up Windows FS semantics, that means the directory cannot be removed / overwritten (unlike posix, where the directory can still be unlinked from the parent).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] StephanEwen commented on issue #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
StephanEwen commented on issue #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#issuecomment-586354299
Interestingly, with proper eager closing of the directory streams, this workaround is also no longer necessary: https://github.com/apache/flink/pull/11095/files#diff-718b96155a7544f30a64f3f389faea3fR67
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#issuecomment-586287248
<!--
Meta data
Hash:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/148979015 TriggerType:PUSH TriggerID:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7
Hash:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=5179 TriggerType:PUSH TriggerID:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7
Hash:0412a4320a08135f4f6c1f580e6eb60b7f763551 Status:PENDING URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=5190 TriggerType:PUSH TriggerID:0412a4320a08135f4f6c1f580e6eb60b7f763551
Hash:0412a4320a08135f4f6c1f580e6eb60b7f763551 Status:PENDING URL:https://travis-ci.com/flink-ci/flink/builds/149011380 TriggerType:PUSH TriggerID:0412a4320a08135f4f6c1f580e6eb60b7f763551
-->
## CI report:
* 1529c3a8abb5e2b758e6a80e2f7359288f87e3d7 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/148979015) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=5179)
* 0412a4320a08135f4f6c1f580e6eb60b7f763551 Travis: [PENDING](https://travis-ci.com/flink-ci/flink/builds/149011380) Azure: [PENDING](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=5190)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] StephanEwen commented on a change in pull request #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
StephanEwen commented on a change in pull request #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#discussion_r379511980
##########
File path: flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/restore/RocksDBIncrementalRestoreOperation.java
##########
@@ -461,26 +457,18 @@ private RestoredDBInstance restoreDBInstanceFromStateHandle(
* a local state.
*/
private void restoreInstanceDirectoryFromPath(Path source, String instanceRocksDBPath) throws IOException {
+ final Path instanceRocksDBDirectory = Paths.get(instanceRocksDBPath);
+ final Path[] files = Files.list(source).toArray(Path[]::new);
Review comment:
The problem with `.map(lambda)` is that some methods in there throw checked exceptions.
I think array/iteration is simpler in the end than exception wrapping / unwrapping.
It is also a rather rare operation, so the occasional array allocation should be fine.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#issuecomment-586287248
<!--
Meta data
Hash:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7 Status:SUCCESS URL:https://travis-ci.com/flink-ci/flink/builds/148979015 TriggerType:PUSH TriggerID:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7
Hash:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7 Status:SUCCESS URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=5179 TriggerType:PUSH TriggerID:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7
-->
## CI report:
* 1529c3a8abb5e2b758e6a80e2f7359288f87e3d7 Travis: [SUCCESS](https://travis-ci.com/flink-ci/flink/builds/148979015) Azure: [SUCCESS](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=5179)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on issue #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#issuecomment-586287248
<!--
Meta data
Hash:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7 Status:PENDING URL:https://travis-ci.com/flink-ci/flink/builds/148979015 TriggerType:PUSH TriggerID:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7
Hash:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7 Status:PENDING URL:https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=5179 TriggerType:PUSH TriggerID:1529c3a8abb5e2b758e6a80e2f7359288f87e3d7
-->
## CI report:
* 1529c3a8abb5e2b758e6a80e2f7359288f87e3d7 Travis: [PENDING](https://travis-ci.com/flink-ci/flink/builds/148979015) Azure: [PENDING](https://dev.azure.com/rmetzger/5bd3ef0a-4359-41af-abca-811b04098d2e/_build/results?buildId=5179)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] carp84 commented on issue #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
carp84 commented on issue #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#issuecomment-586565223
Thanks for the note @StephanEwen, checking now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zentol commented on a change in pull request #11095:
[FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #11095: [FLINK-10918][rocks-db-backend] Fix incremental checkpoints on Windows.
URL: https://github.com/apache/flink/pull/11095#discussion_r379419248
##########
File path: flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/restore/RocksDBIncrementalRestoreOperation.java
##########
@@ -461,26 +457,18 @@ private RestoredDBInstance restoreDBInstanceFromStateHandle(
* a local state.
*/
private void restoreInstanceDirectoryFromPath(Path source, String instanceRocksDBPath) throws IOException {
+ final Path instanceRocksDBDirectory = Paths.get(instanceRocksDBPath);
+ final Path[] files = Files.list(source).toArray(Path[]::new);
Review comment:
this could also be done as a `map` operation on the `Stream<Path>`, then we wouldn't need the array allocation
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services