StephanEwen commented on issue #10768: [FLINK-15012][checkpoint] Introduce shutdown to CheckpointStorageCoordinatorView to clean up checkpoint directory. URL: https://github.com/apache/flink/pull/10768#issuecomment-614768345 Thanks for making another pass over this. Making the cleanup behavior configurable is a compromise, I guess. If many others in the community find this a valuable feature, I am okay with having this, even though I am personally a bit skeptical about features that can lead to "tricky surprises". What I would push for is having very clear docs for this, as part of this PR, so we can be sure we don't forget about them: - A section on the checkpointing docs describing the cleanup behavior, with a big warning that recursive cleanup can lead to problems if - you have savepoints in the same directory as checkpoints - you start a new job with retained checkpoints of another job - The description for the config option should link to the documentation. There are a few code changes I would suggest: (1) Whether cleanup is recursive or now is a feature specific to the file checkpoint storage. The `CheckpointCoordinator` is oblivious to the existence of directories. The division of responsibilities should be as follows: - The `CheckpointCoordinator` descides whether to clean up (compare JobStatus with CheckpointProperties) and calls `storage.shutdownAndCleanup()` if necessary. - The `AbstractFsCheckpointStorage` has the notion what that cleanup means, meaning it has the `isCleanupRecursievFlag`. - The config option should also be scoped to FS storage, not to checkpoints in general. Something like _'state.backend.fs.cleanup.recursive'_. (2) The cleanup is an I/O operation and one that can take very long, so we cannot run it in the main RPC threads. - Especially recursive cleanup can take long, because for many FileSystems/ObjectStores this does effectively mean enumerating all files/objects under the directory/prefix and issuing a separate delete call. Those can be 100s or 1000s, taking many seconds to even minutes. - We could need to run somewhere else, like the I/O executor. Passing an I/O executor to the "shutdown" methods would be a start. - We then have the issue that shutdown does not wait for the operation to be finished, so the Checkpoint Coordinator becomes a component that shuts down asynchronously (shutdown method returns a `CompletableFuture<Void>` that completes when shutdown is complete). That needs to be integrated with `JobMaster` shutdown. - Alternatively, we can try and move the shutdown of the CheckpointStorageLocation to the shutdown of the `JobMaster` which already supports asynchronous shutdown.
---------------------------------------------------------------- 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: us...@infra.apache.org With regards, Apache Git Services
