1996fanrui commented on code in PR #22985:
URL: https://github.com/apache/flink/pull/22985#discussion_r1366420311


##########
flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/adaptive/Executing.java:
##########
@@ -67,13 +77,33 @@ class Executing extends StateWithExecutionGraph implements 
ResourceListener {
         this.context = context;
         Preconditions.checkState(
                 executionGraph.getState() == JobStatus.RUNNING, "Assuming 
running execution graph");
+        this.scalingIntervalMin = scalingIntervalMin;
+        this.scalingIntervalMax = scalingIntervalMax;
+        Preconditions.checkState(
+                !scalingIntervalMin.isNegative(),
+                "{} must be positive integer or 0",
+                JobManagerOptions.SCHEDULER_SCALING_INTERVAL_MIN.key());
+        if (scalingIntervalMax != null) {
+            Preconditions.checkState(
+                    scalingIntervalMax.compareTo(scalingIntervalMin) > 0,
+                    "{}({}) must be greater than {}({})",
+                    JobManagerOptions.SCHEDULER_SCALING_INTERVAL_MAX.key(),
+                    scalingIntervalMax,
+                    JobManagerOptions.SCHEDULER_SCALING_INTERVAL_MIN.key(),
+                    scalingIntervalMin);
+        }
 
         deploy();
 
         // check if new resources have come available in the meantime
         context.runIfState(this, this::maybeRescale, Duration.ZERO);

Review Comment:
   When the `Executing` is created, it means this is a new restart, we still 
need to respect the `scalingIntervalMin`.
   
   If we call `maybeRescale` here, AdaptiveScheduler may not guarantee 
scalingIntervalMin. Following is an example:
   
   - 2 slots comes when job transitions from Creating to Executing.
   - The job will go rescale directly if `maybeRescale` is called, because 2 > 
`min-parallelism-increase`.
   - So this round, the Executing ignore the `scalingIntervalMin`.
   - This can happen all the time when 2 new slots arrive and the job 
transitions from Creating to Executing.
   
   IIUC, when the `Executing` is created, it means this is a new restart, we 
should ensure it can run at least `scalingIntervalMin`.
   
   Please let me know if my understading is wrong or it's not clear.



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

Reply via email to