[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull request #175: [YUNIKORN-241] add request sorting policy

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

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



##########
File path: pkg/scheduler/policies/sorting_policy.go
##########
@@ -26,14 +26,15 @@ import (
 type SortPolicy int
 
 const (
-	FifoSortPolicy   SortPolicy = iota // first in first out, submit time
+	Undefined        SortPolicy = iota // not initialised or parsing failed
+	FifoSortPolicy                     // first in first out, submit time
 	FairSortPolicy                     // fair based on usage
 	StateAwarePolicy                   // only 1 app in starting state
-	Undefined                          // not initialised or parsing failed
+	PriorityPolicy                     // sort on the priority

Review comment:
       this could be confusing. why we are sharing the `SortPolicy` type between apps and requests?

##########
File path: pkg/scheduler/scheduling_allocation_ask.go
##########
@@ -108,9 +108,15 @@ func (saa *schedulingAllocationAsk) getCreateTime() time.Time {
 	return saa.createTime
 }
 
-// Normalised priority
-// Currently a direct conversion.
+// Normalised priority, a direct conversion with a minimal value of 0.
+// 0 is the lowest priority
+// MaxInt the highest.
+// No support for named priorities
 func (saa *schedulingAllocationAsk) normalizePriority(priority *si.Priority) int32 {
 	// TODO, really normalize priority from ask
-	return priority.GetPriorityValue()
+	prioVal := priority.GetPriorityValue()
+	if prioVal < 0 {
+		return 0
+	}

Review comment:
       why we have this assumption that priority must be >= 0?
   I checked some doc such as: https://kubernetes.io/blog/2019/04/16/pod-priority-and-preemption-in-kubernetes/
   
   > If you give a negative priority to your non-critical workload, Cluster Autoscaler does not add more nodes to your cluster when the non-critical pods are pending. Therefore, you won’t incur higher expenses. 




----------------------------------------------------------------
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