Myasuka commented on code in PR #23509: URL: https://github.com/apache/flink/pull/23509#discussion_r1423537299
########## flink-runtime/src/main/java/org/apache/flink/runtime/state/memory/MemoryBackendCheckpointStorageAccess.java: ########## @@ -72,6 +73,7 @@ public MemoryBackendCheckpointStorageAccess( JobID jobId, @Nullable Path checkpointsBaseDirectory, @Nullable Path defaultSavepointLocation, + boolean createCheckpointSubDirs, Review Comment: How about introducing another constructor to make `createCheckpointSubDirs` as default `true`? If so, we can avoid touching many test code in this PR. ########## flink-runtime/src/main/java/org/apache/flink/runtime/state/filesystem/FsStateBackend.java: ########## @@ -141,6 +141,12 @@ public class FsStateBackend extends AbstractFileStateBackend implements Configur */ private final int writeBufferSize; + /** + * Switch to create checkpoint sub-directory with name of jobId. A value of 'undefined' means + * not yet configured, in which case the default will be used. + */ + private TernaryBoolean createCheckpointSubDirs = TernaryBoolean.UNDEFINED; Review Comment: I wonder do we really need to introduce an undefined Boolean here in such way. Currently, we only have one way to set this value within a private constructor: ~~~java this.createCheckpointSubDirs = original.createCheckpointSubDirs.resolveUndefined( configuration.get(CheckpointingOptions.CREATE_CHECKPOINT_SUB_DIS)); ~~~ However, there is no public constructor or setter to set the `createCheckpointSubDirs` in the `original` one, that is to say we will always get a `TernaryBoolean.UNDEFINED` `createCheckpointSubDirs` in the original. -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org