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