xintongsong commented on a change in pull request #11615: [FLINK-16605] Add max
limitation to the total number of slots
URL: https://github.com/apache/flink/pull/11615#discussion_r402744613
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/resourcemanager/slotmanager/SlotManagerImpl.java
##########
@@ -793,16 +799,26 @@ private void
fulfillPendingSlotRequestWithPendingTaskManagerSlot(PendingSlotRequ
return Optional.empty();
}
- private boolean isFulfillableByRegisteredSlots(ResourceProfile
resourceProfile) {
+ private boolean isFulfillableByRegisteredOrPendingSlots(ResourceProfile
resourceProfile) {
for (TaskManagerSlot slot : slots.values()) {
if
(slot.getResourceProfile().isMatching(resourceProfile)) {
return true;
}
}
+
+ for (PendingTaskManagerSlot slot : pendingSlots.values()) {
+ if
(slot.getResourceProfile().isMatching(resourceProfile)) {
+ return true;
+ }
+ }
+
return false;
}
private Optional<PendingTaskManagerSlot>
allocateResource(ResourceProfile resourceProfile) throws
ResourceManagerException {
+ if (getNumberPendingTaskManagerSlots() +
getNumberRegisteredSlots() + numSlotsPerWorker > maxSlotNum) {
+ return Optional.empty();
+ }
Review comment:
I think this makes the SM/RM contract a bit unclear.
Before this PR, SM requests individual slots from RM and is not aware of
`numSlotsPerWorker`.
With this change, SM is aware of slots per worker only for the max slots
limit check, while for other logics (e.g., generating pending slots) it still
pretend not aware of slots per worker.
I would suggest to either move this check to the RM side, or base this PR on
#11320, where we change the contract that SM request workers from RM and is
aware of slots per worker in a consistent way.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
