[GitHub] [incubator-yunikorn-core] yangwwei edited a comment on issue #96: [YUNIKORN-1] Support app/task priority aware scheduling

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

yangwwei edited a comment on issue #96: [YUNIKORN-1] Support app/task priority aware scheduling
URL: https://github.com/apache/incubator-yunikorn-core/pull/96#issuecomment-596344166
 
 
   Hi @TaoYang526 
   
   Thanks for the patch, I like the patch does a good abstraction fo requests, so it only creates small footprints on core-scheduling code path. However, the implementation of `SortableLinkedMap` is a bit over-complex to me. I am thinking if we can simplify this. A few high-level comments:
   
   1. please create `common/maps` for map related files and the `SortableLinkedMap` should have its own file. The layout can look like below
   
   ```
     common/
         maps/
             map.go
             sortablelinkedmap.go
             sortablelinkedmap_test.go
   ```
   For `SortableLinkedMap`
   
   The following func seems like a `compareFunc()`, the name of `isPreFunc()` is a bit confusing.
   ```
   // keep linked entries sorted by the provided isPreFuc function,
   // if not provided, always put the new entry into the tail.
   isPreFunc func(i, j interface{}) bool
   ```
   
   2. I feel the `SortableLinkedMap` can be simplified with the following options
   
   2.1. Remove
   ```
    // function helps to locate the entry in the specified condition.
     matchFunc func(value interface{}) bool
   ```
   this doesn't look like a general thing in a map implementation.
   
   2.2 Remove `isFixedOrder`, why we need this? I saw only `true` value is given for this.
   
   3. for struct `SortedRequests`
   
   ```
   type SortedRequests struct {
       // a map of all requests
       requests map[string]*schedulingAllocationAsk
       // sorted priority groups in the descending order by priority
       sortedPriorityGroups *maps.SortableLinkedMap
   }
   ```
   
   why we need them both? can we remove the 1st `requests` map and keep everything in sync in sortedPriorityGroups ?

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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org