wilfred-s commented on a change in pull request #250:
URL:
https://github.com/apache/incubator-yunikorn-core/pull/250#discussion_r585288093
##########
File path: pkg/scheduler/objects/application.go
##########
@@ -67,7 +67,7 @@ type Application struct {
stateMachine *fsm.FSM // application state machine
stateTimer *time.Timer // timer for state time
execTimeout time.Duration // execTimeout for the
application run
- // appTimer *time.Timer // application run timer
+ placeholderTimer *time.Timer // application run timer
Review comment:
nit: Comment (the description) should be updated
##########
File path: pkg/scheduler/objects/application.go
##########
@@ -225,6 +234,56 @@ func (sa *Application) clearStateTimer() {
zap.String("state", sa.stateMachine.Current()))
}
+func (sa *Application) isWaitingStateTimedOut() bool {
+ return sa.IsWaiting() && sa.stateTimer == nil
+}
+func (sa *Application) initPlaceholderTimer() {
+ if sa.placeholderTimer != nil || !sa.IsAccepted() || sa.execTimeout <=
0 {
+ return
+ }
+ log.Logger().Debug("Application placeholder timer initiated",
+ zap.String("AppID", sa.ApplicationID),
+ zap.Duration("Timeout", sa.execTimeout))
+ sa.placeholderTimer = time.AfterFunc(sa.execTimeout,
sa.timeoutPlaceholderProcessing)
+}
+
+func (sa *Application) clearPlaceholderTimer() {
+ if sa == nil || sa.placeholderTimer == nil {
+ return
+ }
+ sa.placeholderTimer.Stop()
+ sa.placeholderTimer = nil
+}
+
+func (sa *Application) timeoutPlaceholderProcessing() {
+ sa.Lock()
+ defer sa.Unlock()
+ var allocationsToRelease []*Allocation
+ // Case 1: if all app's placeholders are allocated, only part of them
gets replaced, just delete the remaining placeholders
+ switch {
+ case sa.allPlaceholdersAllocated() && !sa.allPlaceholdersReplaced():
+ {
Review comment:
nit: We do not need brackets around the lines within the case.
Indentation should handle it all and it reads easier when they are not there.
##########
File path: pkg/scheduler/objects/application.go
##########
@@ -1015,8 +1072,11 @@ func (sa *Application) GetAllAllocations() []*Allocation
{
// get a copy of all placeholder allocations of the application
// No locking must be called while holding the lock
-func (sa *Application) getPlaceholderAllocations() []*Allocation {
+func (sa *Application) GetPlaceholderAllocations() []*Allocation {
var allocations []*Allocation
+ if sa == nil || len(sa.allocations) == 0 {
+ return allocations
+ }
Review comment:
I am not sure why we export this as it is not used outside the
application itself.
The app reference _sa_ can also never be nil, we would have crashed way
before getting the placeholders allocations.
##########
File path: pkg/scheduler/objects/application.go
##########
@@ -1025,6 +1085,17 @@ func (sa *Application) getPlaceholderAllocations()
[]*Allocation {
return allocations
}
+func (sa *Application) GetAllRequests() []*AllocationAsk {
+ sa.RLock()
+ defer sa.RUnlock()
Review comment:
This is called from `timeoutPlaceholderProcessing()` which is a local
call so we should not export the method. We also take a write lock in that
method so this will deadlock. It must be an unlocked call.
A unit test for `timeoutPlaceholderProcessing()` should be able to pick this
deadlock up.
##########
File path: pkg/scheduler/objects/application.go
##########
@@ -1163,6 +1245,24 @@ func (sa *Application) GetTag(tag string) string {
return tagVal
}
+func (sa *Application) allPlaceholdersAllocated() bool {
+ return resources.Equals(sa.allocatedPlaceholder, sa.placeholderAsk)
+}
+
+func (sa *Application) allPlaceholdersReplaced() bool {
+ return resources.IsZero(sa.allocatedPlaceholder)
Review comment:
this is called once only merge this check into the caller
`timeoutPlaceholderProcessing()`
##########
File path: pkg/scheduler/objects/application.go
##########
@@ -1163,6 +1245,24 @@ func (sa *Application) GetTag(tag string) string {
return tagVal
}
+func (sa *Application) allPlaceholdersAllocated() bool {
+ return resources.Equals(sa.allocatedPlaceholder, sa.placeholderAsk)
+}
+
+func (sa *Application) allPlaceholdersReplaced() bool {
+ return resources.IsZero(sa.allocatedPlaceholder)
+}
+
+func (sa *Application) GetRMID() string {
+ return sa.rmID
+}
Review comment:
This is a method that is not used and should be removed.
##########
File path: pkg/scheduler/objects/application.go
##########
@@ -1163,6 +1245,24 @@ func (sa *Application) GetTag(tag string) string {
return tagVal
}
+func (sa *Application) allPlaceholdersAllocated() bool {
+ return resources.Equals(sa.allocatedPlaceholder, sa.placeholderAsk)
+}
+
+func (sa *Application) allPlaceholdersReplaced() bool {
+ return resources.IsZero(sa.allocatedPlaceholder)
+}
+
+func (sa *Application) GetRMID() string {
+ return sa.rmID
+}
+
+func (sa *Application) GetStateTimer() *time.Timer {
+ sa.RLock()
+ defer sa.RUnlock()
+ return sa.stateTimer
+}
Review comment:
This is a method that is not used and should be removed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
