wilfred-s commented on code in PR #597:
URL: https://github.com/apache/yunikorn-core/pull/597#discussion_r1275697254
##########
pkg/scheduler/objects/application.go:
##########
@@ -1712,6 +1715,17 @@ func (sa *Application) ReplaceAllocation(uuid string)
*Allocation {
if sa.placeholderData != nil {
sa.placeholderData[ph.GetTaskGroup()].Replaced++
}
+
+ // check if we have active placeholder allocation left
+ hasReplaceablePlaceholder := false
+ for _, phData := range sa.placeholderData {
Review Comment:
Better solution is to couple this to the replacement and the placeholder
allocation accounting that we already have: `addAllocationInternal` and
`removeAllocationInternal`
1. Set once: when we initialise the placeholder timeout we set the flag to
true
2. Unset once: when we have removed the last allocated placeholder, same
time as we clear the placeholder timer
##########
pkg/scheduler/objects/application.go:
##########
@@ -2013,6 +2027,10 @@ func (sa *Application) LogAppSummary(rmID string) {
appSummary.ResourceUsage = nil
}
+func (sa *Application) HasPlaceholderAllocation() bool {
+ return sa.hasPlaceholderAlloc.Load()
Review Comment:
See above: a mutex can be used. Setting the boolean happens inside a mutex
locked application. This is the only read point.
##########
pkg/scheduler/objects/application.go:
##########
@@ -103,6 +104,7 @@ type Application struct {
stateLog []*StateLogEntry // state log for this
application
placeholderData map[string]*PlaceholderData // track placeholder
and gang related info
askMaxPriority int32 // highest priority
value of outstanding asks
+ hasPlaceholderAlloc atomic.Bool // Whether there is at
least one allocated placeholder (has to be atomic.Bool due to queue/app lock
ordering)
Review Comment:
Does not need to be atomic, application sorting uses the calls to get other
data from the application which require locking the application. Can use the
same mechanism. Sticking with that makes sure that if a release is in progress
it works.
##########
pkg/scheduler/objects/application.go:
##########
@@ -1712,6 +1715,17 @@ func (sa *Application) ReplaceAllocation(uuid string)
*Allocation {
if sa.placeholderData != nil {
sa.placeholderData[ph.GetTaskGroup()].Replaced++
}
+
+ // check if we have active placeholder allocation left
+ hasReplaceablePlaceholder := false
+ for _, phData := range sa.placeholderData {
+ if phData.Count > (phData.Replaced + phData.TimedOut) {
+ hasReplaceablePlaceholder = true
+ break
+ }
+ }
+ sa.hasPlaceholderAlloc.Store(hasReplaceablePlaceholder)
Review Comment:
See earlier comment, placeholder allocation based is more stable and does
not need any new looping as we already have the checks for other things
--
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]
