[GitHub] [incubator-yunikorn-k8shim] kingamarton commented on a change in pull request #269: YUNIKORN-679: Add gangSchedulingStyle scheduling parameter

[GitBox <gi...@apache.org>]

kingamarton commented on a change in pull request #269:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/269#discussion_r642558630



##########
File path: pkg/appmgmt/general/general.go
##########
@@ -134,12 +134,18 @@ func (os *Manager) getAppMetadata(pod *v1.Pod) (interfaces.ApplicationMetadata,
 	}
 	ownerReferences := getOwnerReferences(pod)
 
-	placeholderTimeout, err := utils.GetPlaceholderTimeoutParam(pod)
-	if err != nil {
+	placeholderTimeout, placeholderTimeoutErr, schedulingStyle, schedulingStyleErr := utils.GetSchedulingPolicyParam(pod)

Review comment:
       this is a bit confusing. Please create 2 different methods (GetPlaceholderTimeoutParam and GetSchedulingStyle) and eventually export the repeated parts into a separate one.

##########
File path: pkg/appmgmt/general/general_test.go
##########
@@ -96,6 +98,9 @@ func TestGetAppMetadata(t *testing.T) {
 				"queue":                    "root.b",
 				"yunikorn.apache.org/user": "testuser",
 			},
+			Annotations: map[string]string{
+				constants.SchedulingPolicyStyleParam: "gangSchedulingStyle=Hard",

Review comment:
       Please export the parameter name in a constant and use that one. Also use constants for the possible values

##########
File path: pkg/callback/scheduler_callback.go
##########
@@ -149,6 +149,16 @@ func (callback *AsyncRMCallback) RecvUpdateResponse(response *si.UpdateResponse)
 			if err != nil {
 				log.Logger().Error("failed to delete application", zap.Error(err))
 			}
+		} else if updated.State == events.States().Application.Resuming {
+			app := callback.context.GetApplication(updated.ApplicationID)
+			if app != nil && app.GetApplicationState() == events.States().Application.Reserving {
+				ev := cache.NewRunApplicationEvent(updated.ApplicationID)
+				dispatcher.Dispatch(ev)
+
+				// handle status update
+				dispatcher.Dispatch(cache.NewApplicationStatusChangeEvent(updated.ApplicationID, events.AppStateChange, updated.State))
+			}

Review comment:
       The core side will move the application into the Resuming state before we have all the placeholders removed. See the changes in the PR: https://github.com/apache/incubator-yunikorn-core/pull/275/files
   
   I think when we need to move this part to when we remove the allocations and asks, to make sure that first we delete all the placeholders. 

##########
File path: pkg/common/utils/gang_utils.go
##########
@@ -123,24 +125,42 @@ func GetTaskGroupsFromAnnotation(pod *v1.Pod) ([]v1alpha1.TaskGroup, error) {
 	return taskGroups, nil
 }
 
-func GetPlaceholderTimeoutParam(pod *v1.Pod) (int64, error) {
+func GetSchedulingPolicyParam(pod *v1.Pod) (int64, error, string, error) {

Review comment:
       Please use two methods instead of mixing the 2 calls. You can export the common parts into one method. See my comment in the general.go file




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