KarmaGYZ commented on code in PR #23201:
URL: https://github.com/apache/flink/pull/23201#discussion_r1294245482
##########
flink-runtime/src/test/java/org/apache/flink/runtime/resourcemanager/slotmanager/DeclarativeSlotManagerBuilder.java:
##########
@@ -45,6 +45,7 @@ public class DeclarativeSlotManagerBuilder {
private WorkerResourceSpec defaultWorkerResourceSpec;
private int numSlotsPerWorker;
private SlotManagerMetricGroup slotManagerMetricGroup;
+ private int minSlotNum;
Review Comment:
We are dropping the DSM. There is no need to take it into account.
##########
flink-runtime/src/main/java/org/apache/flink/runtime/resourcemanager/slotmanager/SlotManagerConfiguration.java:
##########
@@ -178,6 +199,7 @@ public static SlotManagerConfiguration fromConfiguration(
int numSlotsPerWorker =
configuration.getInteger(TaskManagerOptions.NUM_TASK_SLOTS);
+ int minSlotNum =
configuration.getInteger(ResourceManagerOptions.MIN_SLOT_NUM);
Review Comment:
We need to do some sanity check here, including:
- minSlotNum <= maxSlotNum
- there is x where minSlotNum <= x * defaultSlotNumPerTM <= maxSlotNum
- minCPU/Mem <= maxCPU/Mem
- there is x where minCPU/Mem <= x * defaultCPU/MemPerTM <= maxCPU/Mem
##########
flink-runtime/src/main/java/org/apache/flink/runtime/resourcemanager/slotmanager/SlotManagerConfiguration.java:
##########
@@ -88,8 +88,9 @@ public SlotManagerConfiguration(
this.evenlySpreadOutSlots = evenlySpreadOutSlots;
this.defaultWorkerResourceSpec =
Preconditions.checkNotNull(defaultWorkerResourceSpec);
Preconditions.checkState(numSlotsPerWorker > 0);
- Preconditions.checkState(maxSlotNum > 0);
this.numSlotsPerWorker = numSlotsPerWorker;
+ Preconditions.checkState(maxSlotNum > 0);
+ Preconditions.checkState(maxSlotNum >= minSlotNum);
Review Comment:
Do not belong to this commit.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/resourcemanager/slotmanager/SlotManagerConfigurationTest.java:
##########
@@ -68,6 +68,24 @@ void testPreferLegacySlotRequestTimeout() throws Exception {
.isEqualTo(slotManagerConfiguration.getSlotRequestTimeout().toMilliseconds());
}
+ @Test
Review Comment:
Maybe also add a test for conflict min/max value.
--
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]
