manirajv06 commented on code in PR #1127:
URL: https://github.com/apache/yunikorn-core/pull/1127#discussion_r3893572930
##########
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:
From safety perspective, both approaches should not cause any issues as
scheduling cycle itself is sequential in nature - not a parallel processing. As
long as only one loop is process the `sortedRequests` sequentially, we should
be good.
From performance perspective, Since there is extra clone() operation and
most importantly the way each item has been accessed determines the outcome. I
think performance numbers might change if we access the element directly with
index in `for ... range abc.clone()` loop. Something to try out for.
--
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]
