[GitHub] [flink] dawidwys commented on a change in pull request #17179: [FLINK-24086][checkpoint] Rebuilding the SharedStateRegistry only when the restore method is called for the first time.

[GitBox <gi...@apache.org>]

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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org