Myasuka commented on a change in pull request #18157:
URL: https://github.com/apache/flink/pull/18157#discussion_r785936019
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/state/filesystem/FsCheckpointMetadataOutputStream.java
##########
@@ -162,4 +164,97 @@ public FsCompletedCheckpointStorageLocation
closeAndFinalizeCheckpoint() throws
}
}
}
+
+ static MetadataOutputStreamBackend getOutputStreamBackend(
+ final FileSystem fileSystem, final Path metadataFilePath) throws
IOException {
+ try {
+ RecoverableWriter recoverableWriter =
fileSystem.createRecoverableWriter();
+ if (fileSystem.exists(metadataFilePath)) {
+ throw new IOException("Target file " + metadataFilePath + " is
already exists.");
+ }
+ return new
RecoverableStreamBackend(recoverableWriter.open(metadataFilePath));
+ } catch (Throwable throwable) {
+ LOG.warn("Errors on creating recoverable writer.", throwable);
+ }
+ return new FSDataOutputStreamBackend(fileSystem, metadataFilePath);
+ }
+
+ /** The backend to wrap a whole output stream. */
+ interface MetadataOutputStreamBackend {
+
+ FSDataOutputStream getOutput();
+
+ void commit() throws IOException;
Review comment:
Actually, we cannot call `commit` and `close` twice for same stream, and
not all file systems can support close on the same stream twice.
I suggest to rename this method to `closeForCommit`.
Moreover, `streamBackend` looks too close to `stateBackend`. How about
rename it to something like `...wrapper`?
Last but not least, I prefer to add some comments for each method.
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/state/filesystem/FsCheckpointMetadataOutputStream.java
##########
@@ -162,4 +164,97 @@ public FsCompletedCheckpointStorageLocation
closeAndFinalizeCheckpoint() throws
}
}
}
+
+ static MetadataOutputStreamBackend getOutputStreamBackend(
+ final FileSystem fileSystem, final Path metadataFilePath) throws
IOException {
+ try {
+ RecoverableWriter recoverableWriter =
fileSystem.createRecoverableWriter();
+ if (fileSystem.exists(metadataFilePath)) {
+ throw new IOException("Target file " + metadataFilePath + " is
already exists.");
+ }
+ return new
RecoverableStreamBackend(recoverableWriter.open(metadataFilePath));
+ } catch (Throwable throwable) {
+ LOG.warn("Errors on creating recoverable writer.", throwable);
+ }
+ return new FSDataOutputStreamBackend(fileSystem, metadataFilePath);
+ }
+
+ /** The backend to wrap a whole output stream. */
+ interface MetadataOutputStreamBackend {
+
+ FSDataOutputStream getOutput();
+
+ void commit() throws IOException;
+
+ void close() throws IOException;
+
+ void finalize() throws IOException;
Review comment:
`finalize()` sounds like we will call it in the end (compared with
`CheckpointMetadataOutputStream#closeAndFinalizeCheckpoint`). I think method
such as `abort()` could be better.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
