wilfred-s commented on code in PR #384:
URL: https://github.com/apache/yunikorn-core/pull/384#discussion_r843612422
##########
pkg/scheduler/objects/application_state.go:
##########
@@ -172,6 +175,9 @@ func NewAppState() *fsm.FSM {
},
fmt.Sprintf("leave_%s", Running.String()): func(event
*fsm.Event) {
app := event.Args[0].(*Application)
//nolint:errcheck
+ if app.queue != nil {
Review Comment:
The app must have a queue set if it is in running state, there is no need
for a nil check
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1303,42 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x,
MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime,
sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
Review Comment:
Should this increment and decrement below be using a lock on update?
Do need to make sure we do not lock the whole hierarchy, see
incPendingResource for example.
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1303,42 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x,
MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime,
sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ sq.runningApps++
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+}
+
+func (sq *Queue) decRunningApps() {
+ sq.runningApps--
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+}
+
+func (sq *Queue) canRun() bool {
+ sq.RLock()
+ defer sq.RUnlock()
Review Comment:
This will lock the current queue and then walk up the tree, locking all
parents.
Please follow the same structure as we have for all over: first walk up the
tree, check the top and then when unwinding check the current queue. Locking
only when the current queue is checked (see getHeadRoom as an example)
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1303,42 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x,
MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime,
sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ sq.runningApps++
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+}
+
+func (sq *Queue) decRunningApps() {
+ sq.runningApps--
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+}
+
+func (sq *Queue) canRun() bool {
+ sq.RLock()
+ defer sq.RUnlock()
+ if sq.maxRunningApps == 0 {
+ return true
+ }
+ ok := sq.runningApps <= sq.maxRunningApps
+ if sq.parent != nil {
+ return ok && sq.parent.canRun()
+ }
+ return ok
+}
+
+func (sq *Queue) GetRunningApps() uint64 {
+ sq.RLock()
+ defer sq.RUnlock()
+ return sq.runningApps
+}
+
+func (sq *Queue) SetMaxRunningApps(maxApps uint64) {
+ sq.RLock()
+ defer sq.RUnlock()
Review Comment:
This is a read lock securing a write action...
Also please comment that this is test only as it is not called from anywhere
else.
##########
pkg/scheduler/objects/utilities_test.go:
##########
@@ -35,25 +35,27 @@ const (
appID2 = "app-2"
aKey = "alloc-1"
nodeID1 = "node-1"
+ zero = uint64(0)
)
// Create the root queue, base for all testing
func createRootQueue(maxRes map[string]string) (*Queue, error) {
- return createManagedQueueWithProps(nil, "root", true, maxRes, nil)
+ return createManagedQueueWithProps(nil, "root", true, maxRes, nil, zero)
}
// wrapper around the create call using the one syntax for all queue types
-func createManagedQueue(parentSQ *Queue, name string, parent bool, maxRes
map[string]string) (*Queue, error) {
- return createManagedQueueWithProps(parentSQ, name, parent, maxRes, nil)
+func createManagedQueue(parentSQ *Queue, name string, parent bool, maxRes
map[string]string, maxApps uint64) (*Queue, error) {
Review Comment:
Instead of changing this signature add a new function that allows setting
max apps and leave this as is. i.e. `createManagedQueueMaxApps(....)` calling
the next one with a 0 value
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1067,6 +1073,11 @@ func (sq *Queue) TryAllocate(iterator func()
NodeIterator) *Allocation {
headRoom := sq.getHeadRoom()
// process the apps (filters out app without pending requests)
for _, app := range sq.sortApplications(true) {
+ if !app.IsRunning() && !app.queue.canRun() {
Review Comment:
You are in the queue, should not access the app for this info. It also does
not change while walking over the application list. You can pre-calculate the
value and use it.
##########
pkg/scheduler/objects/utilities_test.go:
##########
@@ -35,25 +35,27 @@ const (
appID2 = "app-2"
aKey = "alloc-1"
nodeID1 = "node-1"
+ zero = uint64(0)
)
// Create the root queue, base for all testing
func createRootQueue(maxRes map[string]string) (*Queue, error) {
- return createManagedQueueWithProps(nil, "root", true, maxRes, nil)
+ return createManagedQueueWithProps(nil, "root", true, maxRes, nil, zero)
}
// wrapper around the create call using the one syntax for all queue types
-func createManagedQueue(parentSQ *Queue, name string, parent bool, maxRes
map[string]string) (*Queue, error) {
- return createManagedQueueWithProps(parentSQ, name, parent, maxRes, nil)
+func createManagedQueue(parentSQ *Queue, name string, parent bool, maxRes
map[string]string, maxApps uint64) (*Queue, error) {
+ return createManagedQueueWithProps(parentSQ, name, parent, maxRes, nil,
maxApps)
}
// create managed queue with props set
-func createManagedQueueWithProps(parentSQ *Queue, name string, parent bool,
maxRes, props map[string]string) (*Queue, error) {
+func createManagedQueueWithProps(parentSQ *Queue, name string, parent bool,
maxRes, props map[string]string, maxApps uint64) (*Queue, error) {
Review Comment:
Instead of changing this signature add a new function that allows setting
max apps and leave this as is. i.e. `createManagedQueuePropsMaxApps(....)`
Move the real code there and make `createManagedQueueWithProps()` call that
one
##########
pkg/scheduler/objects/application_state.go:
##########
@@ -143,6 +143,9 @@ func NewAppState() *fsm.FSM {
},
fmt.Sprintf("enter_%s", Starting.String()): func(event
*fsm.Event) {
app := event.Args[0].(*Application)
//nolint:errcheck
+ if app.queue != nil {
Review Comment:
The app must have a queue set if it is in starting state, there is no need
for a nil check
--
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]
