[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull request #275: [YUNIKORN-582] Introduced soft/hard gang scheduling styles

[GitBox]

yangwwei commented on a change in pull request #275:
URL: 
https://github.com/apache/incubator-yunikorn-core/pull/275#discussion_r644405520



##########
File path: pkg/scheduler/objects/application.go
##########
@@ -312,9 +330,14 @@ func (sa *Application) timeoutPlaceholderProcessing() {
                log.Logger().Info("Placeholder timeout, releasing asks and 
placeholders",
                        zap.String("AppID", sa.ApplicationID),
                        zap.Int("releasing placeholders", len(sa.allocations)),
-                       zap.Int("releasing asks", len(sa.requests)))
+                       zap.Int("releasing asks", len(sa.requests)),
+                       zap.String("used Scheduling type", 
sa.gangSchedulingStyle))

Review comment:
       suggest to change "used scheduling type" to "gang scheduling style", 
just to keep the words consistent

##########
File path: pkg/scheduler/objects/application.go
##########
@@ -97,6 +104,13 @@ func NewApplication(siApp *si.AddApplicationRequest, ugi 
security.UserGroup, eve
        if time.Duration(0) == placeholderTimeout {
                placeholderTimeout = defaultPlaceholderTimeout
        }
+       gangSchedStyle := siApp.GetGangSchedulingStyle()
+       if gangSchedStyle != Soft && gangSchedStyle != Hard {
+               log.Logger().Info("Unknown gang scheduling style, using hard 
style as default",
+                       zap.String("gang scheduling style", gangSchedStyle))
+               gangSchedStyle = Hard
+       }

Review comment:
       we should use `soft` as default

##########
File path: pkg/scheduler/objects/application_state.go
##########
@@ -106,6 +108,10 @@ func NewAppState() *fsm.FSM {
                                Name: FailApplication.String(),
                                Src:  []string{Failing.String()},
                                Dst:  Failed.String(),
+                       }, {
+                               Name: ResumeApplication.String(),
+                               Src:  []string{New.String(), Accepted.String()},
+                               Dst:  Resuming.String(),

Review comment:
       <img width="1118" 
alt="YuniKorn__Online_Whiteboard_for_Visual_Collaboration" 
src="https://user-images.githubusercontent.com/14214570/120569352-94ab5a80-c3ca-11eb-86ff-7ef94070ba7e.png";>
   
   in this chart, I think when placeholder times out, the app could be in any 
state among [Accepted, Starting, Running, Waiting, Completing]. But we only 
handle the states `New` and `Accepted`, will this be able to cover all other 
cases?




-- 
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:
us...@infra.apache.org