pnowojski commented on a change in pull request #9885:
[FLINK-14344][checkpointing] Snapshots master hook state asynchronously
URL: https://github.com/apache/flink/pull/9885#discussion_r343070065
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
##########
@@ -659,10 +660,10 @@ else if (!props.forceCheckpoint()) {
}
// trigger the master hooks for the checkpoint
- final List<MasterState> masterStates =
MasterHooks.triggerMasterHooks(masterHooks.values(),
+ final Map<String, MasterState> masterStates =
MasterHooks.triggerMasterHooks(masterHooks.values(),
Review comment:
I'm a bit confused by this first commit (`[FLINK-14344][checkpointing]
PendingCheckpoint supports acknowledgin master state`).
This commit just adds `isMasterStatesFullyAcknowledged()` method, which at
least in this PR, is only used in couple of `checkState()` calls, right?
- maybe it needs to be squashed with something (or should be introduced in
some future PR?), otherwise it's confusing the readers
- if not squashed, it requires some explanation in the commit message
The title itself is also a bit confusing, as `PendingCheckpoint` was already
supporting acknowledging of master states even without this commit, right?
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
