You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@yunikorn.apache.org by GitBox <gi...@apache.org> on 2020/03/04 10:39:53 UTC
[GitHub] [incubator-yunikorn-core] TaoYang526 opened a new pull request #96:
[YUNIKORN-1] Support app/task priority aware scheduling
TaoYang526 opened a new pull request #96: [YUNIKORN-1] Support app/task priority aware scheduling
URL: https://github.com/apache/incubator-yunikorn-core/pull/96
This is an initial implementation based on the latest design doc in YUNIKORN-1
@yangwwei @wilfred-s could you please help to review this PR? Thanks.
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] TaoYang526 commented on issue #96:
[YUNIKORN-1] Support app/task priority aware scheduling
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #96: [YUNIKORN-1] Support app/task priority aware scheduling
URL: https://github.com/apache/incubator-yunikorn-core/pull/96#issuecomment-598554445
@yangwwei @wilfred-s
To confirm that the performance won't regress unexpectedly, I have added benchmark tests for apps sorting and pending requests iteration in latest PR.
For apps sorting, it tests the performance of sorting pending apps with different policies, test results in my local environment like this (unit of consuming-time is µs):
| | 10 apps | 100 apps | 1000 apps
-- | -- | -- | -- | --
Before | Fair | 8.9 | 118.9 | 1129.0
| Fifo | 3.9 | 44.2 | 657.8
After | Fair | 8.6 | 125.2 | 1203.0
| Fifo | 4.7 | 81.2 | 1483.0
There is little change for Fair policy, and there's not too much change for Fifo policy, which is expected since now Fifo has more things to be considered.
For pending requests iteration, it tests get and iterate 10,000 sorted pending requests with different number of priorities, expect the performance is only related to the number of pending requests, won't be affected by other factors such as the number of priorities and pending percentage, and should have better performance than before, test results in my local environment like this (unit of consuming-time is µs):
| 1 priority | 2 priorities | 5 priorities | 10 priorities
-- | -- | -- | -- | --
Before | 883.4 | 1642.5 | 2774.3 | 3246.9
After | 288.7 | 279.9 | 286.3 | 292.3
According to the results, it will improve a lot than before, at least 3 times enhancement and more priorities leads to more enhancement. Moreover, only priority is considered for sorting before, if it supports sorting by createTime as in this PR, the consuming-time will be much more(about 6ms for 10,000 requests).
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] TaoYang526 commented on issue #96:
[YUNIKORN-1] Support app/task priority aware scheduling
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #96: [YUNIKORN-1] Support app/task priority aware scheduling
URL: https://github.com/apache/incubator-yunikorn-core/pull/96#issuecomment-597674600
Addressed comments from @yangwwei and @wangdatan.
Updates in the latest commit:
(1) Remove resort & isFixedOrder logic to simplify SortableLinkedMap.
(2) Merge PriorityFifo and Fifo policies.
(3) Support validating configured app-sort-policy.
(4) Update UTs.
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] yangwwei edited a comment on issue #96:
[YUNIKORN-1] Support app/task priority aware scheduling
Posted by 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
[GitHub] [incubator-yunikorn-core] TaoYang526 edited a comment on issue #96:
[YUNIKORN-1] Support app/task priority aware scheduling
Posted by GitBox <gi...@apache.org>.
TaoYang526 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-596368653
Thanks @yangwwei for the review and comments.
Replies about the comments above:
1. LGTM
2. (1) matchFunc helps to locate the first pending request so that we can quickly find out the top priority group and the first pending request in a priority group, otherwise we have to iterate all requests to find pending request for every cycle.
(2)isFixedOrder can be used for different scenarios, for now the priority of request can't be modified so that we can set isFixedOrder to true to avoid reordering this request in the map, but if we support updating the priority in the future, we can set isFixedOrder to false. But it's fine for me to move it and not support resorting when updating the entry.
3. We have lots of operations need to get request by allocationKey, if not keep all requests in map, we have to find it from priority groups, perhaps it's fine to keep references in several places for better performance?
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] TaoYang526 commented on issue #96:
[YUNIKORN-1] Support app/task priority aware scheduling
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #96: [YUNIKORN-1] Support app/task priority aware scheduling
URL: https://github.com/apache/incubator-yunikorn-core/pull/96#issuecomment-596368653
Thanks @yangwwei for the review and comments.
Replay about the comments
1. LGTM
2.1 matchFunc helps to locate the first pending request so that we can quickly find out the top priority group and the first pending request in a priority group, otherwise we have to iterate all requests to find pending request for every cycle.
2.2 isFixedOrder can be used for different scenarios, for now the priority of request can't be modified so that we can set isFixedOrder to true to avoid reordering this request in the map, but if we support updating the priority in the future, we can set isFixedOrder to false. But it's fine for me to move it and not support resorting when updating the entry.
3 We have lots of operations need to get request by allocationKey, if not keep all requests in map, we have to find it from priority groups, perhaps it's fine to keep references in several places for better performance?
----------------------------------------------------------------
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
[GitHub] [incubator-yunikorn-core] yangwwei commented on issue #96:
[YUNIKORN-1] Support app/task priority aware scheduling
Posted by GitBox <gi...@apache.org>.
yangwwei commented 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.
```
isPreFunc func(i, j interface{}) bool
```
2. I feel the `SortableLinkedMap` can be simplified with the following options
2.1
----------------------------------------------------------------
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