RocMarshal commented on code in PR #25218:
URL: https://github.com/apache/flink/pull/25218#discussion_r1775043513
##########
flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/adaptive/allocator/DefaultSlotAssigner.java:
##########
@@ -43,35 +47,43 @@ public Collection<SlotAssignment> assignSlots(
Collection<? extends SlotInfo> freeSlots,
VertexParallelism vertexParallelism,
JobAllocationsInformation previousAllocations) {
- List<ExecutionSlotSharingGroup> allGroups = new ArrayList<>();
+ checkSlotsSufficient(jobInformation, freeSlots);
+
+ final List<ExecutionSlotSharingGroup> allGroups = new ArrayList<>();
for (SlotSharingGroup slotSharingGroup :
jobInformation.getSlotSharingGroups()) {
allGroups.addAll(createExecutionSlotSharingGroups(vertexParallelism,
slotSharingGroup));
}
- Iterator<? extends SlotInfo> iterator = freeSlots.iterator();
+ Collection<? extends SlotInfo> pickedSlots = freeSlots;
+ if (freeSlots.size() > allGroups.size()) {
Review Comment:
Thanks for the comments.
>Why do we need this condition? To avoid the extra effort of sorting the TMs
based on their offered slot count? If yes, could we add this as a comment to
underline the intention?
Yes, you are right. I'd like to update it.
>On the other hand, is it worth to save the sorting efforts if
freeSlots.size() == allGroups.size()? 🤔 I guess it would make sense for big
amounts of TaskManagers.
Yes, so we keep the condition `if (freeSlots.size() > allGroups.size()) {` ,
which means that this condition you described would not process the sort-logic.
> I don't know the limits we have on that front in Flink at the moment.
In my limited read, nothing limited about it.
--
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]
