dawidwys commented on a change in pull request #17179:
URL: https://github.com/apache/flink/pull/17179#discussion_r731679412
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointRecoveryFactory.java
##########
@@ -31,10 +32,15 @@
* @param jobId Job ID to recover checkpoints for
* @param maxNumberOfCheckpointsToRetain Maximum number of checkpoints to
retain
* @param userClassLoader User code class loader of the job
+ * @param sharedStateRegistry Registry that tracks state which is shared
across (incremental)
+ * checkpoints
* @return {@link CompletedCheckpointStore} instance for the job
*/
CompletedCheckpointStore createRecoveredCompletedCheckpointStore(
- JobID jobId, int maxNumberOfCheckpointsToRetain, ClassLoader
userClassLoader)
+ JobID jobId,
+ int maxNumberOfCheckpointsToRetain,
+ ClassLoader userClassLoader,
+ SharedStateRegistry sharedStateRegistry)
Review comment:
I don't think it is a good idea. It does not define a clear contract for
the `SharedStateRegistry`. Is it empty? Does it have entries? What should we do
about it if it is not empty?
It should be up to the `CheckpointRecoveryFactory` to tell where does the
`SharedStateRegistry` comes from.
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CompletedCheckpointStore.java
##########
@@ -105,11 +108,16 @@ default long getLatestCheckpointId() {
*/
boolean requiresExternalizedCheckpoints();
+ void registerSharedState(Map<OperatorID, OperatorState> operatorStates);
Review comment:
This mixes responsibilities of the two classes/interfaces
(`CompletedCheckpointStore` & `SharedStateRegistry`). I am not against coupling
those two (as they're lifecycles are coupled already), but not in this way.
Maybe it would make sense to add `getSharedStateRegistry()`. I guess we
would need to extract an interface from `SharedStateRegistry` then.
--
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]
