[GitHub] [flink] xintongsong commented on a change in pull request #16640: [FLINK-22891][runtime] Using CompletableFuture to sync the scheduling…

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

xintongsong commented on a change in pull request #16640:
URL: https://github.com/apache/flink/pull/16640#discussion_r681377850



##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/resourcemanager/slotmanager/FineGrainedSlotManager.java
##########
@@ -111,6 +111,8 @@
 
     @Nullable private ScheduledFuture<?> lastResourceRequirementsCheck;
 
+    @Nullable private CompletableFuture<Void> requirementsCheckFuture;

Review comment:
       I wonder if we can remove `lastResourceRequirementsCheck`. Currently, it is only used for canceling the check on closing. Alternatively, we can check `started` in `checkResourceRequirements`.

##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/resourcemanager/slotmanager/FineGrainedSlotManager.java
##########
@@ -550,6 +555,7 @@ private void checkResourceRequirements() {
                         jobId, resourceTracker.getAcquiredResources(jobId));
             }
         }
+        requirementsCheckFuture.complete(null);

Review comment:
       And maybe add a null check for `requirementsCheckFuture`.

##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/resourcemanager/slotmanager/FineGrainedSlotManager.java
##########
@@ -550,6 +555,7 @@ private void checkResourceRequirements() {
                         jobId, resourceTracker.getAcquiredResources(jobId));
             }
         }
+        requirementsCheckFuture.complete(null);

Review comment:
       I'd suggest to deduplicate this by moving it to `checkResourceRequirementsWithDelay`, after calling `checkResourceRequirements`.




-- 
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