tigerquoll commented on code in PR #1127:
URL: https://github.com/apache/yunikorn-core/pull/1127#discussion_r3888382473
##########
pkg/scheduler/objects/application.go:
##########
@@ -1358,7 +1412,11 @@ func (sa *Application)
tryPlaceholderAllocate(nodeIterator func() NodeIterator,
var phFit *Allocation
var reqFit *Allocation
// get all the requests from the app sorted in order
- for _, request := range sa.sortedRequests {
+ // NOTE: iterate over a snapshot, not sa.sortedRequests directly. The
revert path below calls
+ // sa.deallocateAsk(request), which re-inserts the ask into
sa.sortedRequests mid-range - mutating
+ // the live slice while ranging over it would skip/revisit entries.
allocateAsk/deallocateAsk keep
+ // only pending asks in sortedRequests.
+ for _, request := range slices.Clone(sa.sortedRequests) {
Review Comment:
I think `tryPlaceholderAllocate` should keep its clone: its revert path
re-inserts into the slice mid-loop, which an index loop cannot tolerate. . For
`tryAllocate` see the thread above — the clone measured at −11%, so I am
suggesting it uses the index loop.
--
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]
