1996fanrui commented on code in PR #23635:
URL: https://github.com/apache/flink/pull/23635#discussion_r1427526869
##########
flink-runtime/src/main/java/org/apache/flink/runtime/jobmaster/slotpool/DefaultDeclarativeSlotPool.java:
##########
@@ -103,12 +106,35 @@ public class DefaultDeclarativeSlotPool implements
DeclarativeSlotPool {
private final RequirementMatcher requirementMatcher = new
DefaultRequirementMatcher();
+ // For batch slots requests
+ @Nullable private final Time slotRequestMaxInterval;
+ @Nullable private final ComponentMainThreadExecutor
componentMainThreadExecutor;
Review Comment:
If we introduce `ComponentMainThreadExecutor`, I prefer it's `@Nonnull` even
if `slotRequestMaxInterval` is null.
This thread executor may be used for other scenarios, it would be better not
bound to`slotRequestMaxInterval`.
##########
flink-runtime/src/main/java/org/apache/flink/runtime/jobmaster/slotpool/DefaultDeclarativeSlotPool.java:
##########
@@ -103,12 +106,35 @@ public class DefaultDeclarativeSlotPool implements
DeclarativeSlotPool {
private final RequirementMatcher requirementMatcher = new
DefaultRequirementMatcher();
+ // For batch slots requests
+ @Nullable private final Time slotRequestMaxInterval;
+ @Nullable private final ComponentMainThreadExecutor
componentMainThreadExecutor;
+ @Nullable private ScheduledFuture<?> slotRequestMaxIntervalTimeoutFuture;
+
public DefaultDeclarativeSlotPool(
JobID jobId,
AllocatedSlotPool slotPool,
Consumer<? super Collection<ResourceRequirement>>
notifyNewResourceRequirements,
Time idleSlotTimeout,
Time rpcTimeout) {
+ this(
+ jobId,
+ slotPool,
+ notifyNewResourceRequirements,
+ idleSlotTimeout,
+ rpcTimeout,
+ null,
+ null);
+ }
+
+ public DefaultDeclarativeSlotPool(
+ JobID jobId,
+ AllocatedSlotPool slotPool,
+ Consumer<? super Collection<ResourceRequirement>>
notifyNewResourceRequirements,
+ Time idleSlotTimeout,
+ Time rpcTimeout,
+ @Nullable Time slotRequestMaxInterval,
+ @Nullable ComponentMainThreadExecutor componentMainThreadExecutor)
{
Review Comment:
They have been marked to `@Nullable`, do we still need 2 constructors?
May be one constructor with full parameters is enough.
Note: `BlocklistDeclarativeSlotPool` is same.
Also, this commit is a part of FLINK-33388, right? Why doesn't it introduce
`slotRequestMaxInterval` in this commit? I didn't see any caller pass
`slotRequestMaxInterval` in this PR.
`commit message` is better to adding the corresponding JIRA id.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
