You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2021/09/01 09:34:10 UTC

[GitHub] [incubator-yunikorn-core] chia7712 opened a new pull request #318: [YUNIKORN-831] node sorting should check other resources of nodes ins…

chia7712 opened a new pull request #318:
URL: https://github.com/apache/incubator-yunikorn-core/pull/318


   …tead of comparing node id directly
   
   ### What is this PR for?
   Node sorting scores node according to `resources.LargestUsageShare`. However, the largest usage in our cluster is always storage (and all nodes have same storage capacity). It results in the order is always based on `node id`. Before this patch, the order will compare remaining "usage" (vcore, memory, etc) when largest usage is equal.
   
   
   ### What type of PR is it?
   * [x] - Bug Fix
   * [ ] - Improvement
   * [ ] - Feature
   * [ ] - Documentation
   * [ ] - Hot Fix
   * [ ] - Refactoring
   
   ### Todos
   * [ ] - Task
   
   ### What is the Jira issue?
   https://issues.apache.org/jira/browse/YUNIKORN-831
   
   ### How should this be tested?
   
   ### Screenshots (if appropriate)
   
   ### Questions:
   * [ ] - The licenses files need update.
   * [ ] - There is breaking changes for older versions.
   * [ ] - It needs documentation.
   


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

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] codecov[bot] commented on pull request #318: [YUNIKORN-831] node sorting should check other resources of nodes ins…

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #318:
URL: https://github.com/apache/incubator-yunikorn-core/pull/318#issuecomment-909058455


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#318](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c01180f) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5a1c19e) will **increase** coverage by `2.65%`.
   > The diff coverage is `65.57%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #318      +/-   ##
   ==========================================
   + Coverage   63.46%   66.12%   +2.65%     
   ==========================================
     Files          60       63       +3     
     Lines        5220     5934     +714     
   ==========================================
   + Hits         3313     3924     +611     
   - Misses       1747     1808      +61     
   - Partials      160      202      +42     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [pkg/metrics/init.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvaW5pdC5nbw==) | `62.96% <ø> (ø)` | |
   | [pkg/scheduler/drf\_preemption\_policy.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9kcmZfcHJlZW1wdGlvbl9wb2xpY3kuZ28=) | `0.00% <0.00%> (ø)` | |
   | [pkg/scheduler/nodes\_usage\_monitor.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9ub2Rlc191c2FnZV9tb25pdG9yLmdv) | `0.00% <0.00%> (ø)` | |
   | [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
   | [pkg/scheduler/objects/node\_iterator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGVfaXRlcmF0b3IuZ28=) | `100.00% <ø> (ø)` | |
   | [pkg/scheduler/objects/sorters.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3NvcnRlcnMuZ28=) | `100.00% <ø> (ø)` | |
   | [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `32.25% <ø> (+19.35%)` | :arrow_up: |
   | [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `16.10% <23.33%> (+9.84%)` | :arrow_up: |
   | [pkg/metrics/metrics\_collector.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvbWV0cmljc19jb2xsZWN0b3IuZ28=) | `64.51% <45.00%> (-9.40%)` | :arrow_down: |
   | [pkg/metrics/scheduler.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3Mvc2NoZWR1bGVyLmdv) | `58.92% <49.33%> (-2.66%)` | :arrow_down: |
   | ... and [27 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [66cdb3d...c01180f](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


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

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] craigcondit commented on pull request #318: [YUNIKORN-831] node sorting should check other resources of nodes instead of comparing node id directly

Posted by GitBox <gi...@apache.org>.
craigcondit commented on pull request #318:
URL: https://github.com/apache/incubator-yunikorn-core/pull/318#issuecomment-920315442


   Upon testing this locally, it seems that we really do need to consider the fact that resources have different units. I've opened a new issue https://issues.apache.org/jira/browse/YUNIKORN-847 with PR https://github.com/apache/incubator-yunikorn-core/pull/323 that provides an alternative approach. The new PR uses configurable weights for each resource and normalizes resources into percentage occupied rather than considering the absolute values. By default, weights are equal for vcore and memory, and 0 for everything else (effectively ignoring them).


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

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] craigcondit commented on a change in pull request #318: [YUNIKORN-831] node sorting should check other resources of nodes ins…

Posted by GitBox <gi...@apache.org>.
craigcondit commented on a change in pull request #318:
URL: https://github.com/apache/incubator-yunikorn-core/pull/318#discussion_r699356820



##########
File path: pkg/scheduler/objects/node_collection.go
##########
@@ -211,13 +213,7 @@ func (nc *baseNodeCollection) NodeUpdated(node *Node) {
 	nc.Lock()
 	defer nc.Unlock()
 
-	nref := nc.nodes[node.NodeID]
-	if nref == nil {
-		return
-	}
-
-	updatedScore := nc.scoreNode(node)
-	if nref.nodeScore != updatedScore {
+	if nref, ok := nc.nodes[node.NodeID]; ok {

Review comment:
       Actually, I may be mistaken. It looks like you're unconditionally removing and re-adding the node? If so, that also works.




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

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] chia7712 closed pull request #318: [YUNIKORN-831] node sorting should check other resources of nodes instead of comparing node id directly

Posted by GitBox <gi...@apache.org>.
chia7712 closed pull request #318:
URL: https://github.com/apache/incubator-yunikorn-core/pull/318


   


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

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] chia7712 commented on a change in pull request #318: [YUNIKORN-831] node sorting should check other resources of nodes ins…

Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #318:
URL: https://github.com/apache/incubator-yunikorn-core/pull/318#discussion_r699402769



##########
File path: pkg/scheduler/objects/node_collection.go
##########
@@ -211,13 +213,7 @@ func (nc *baseNodeCollection) NodeUpdated(node *Node) {
 	nc.Lock()
 	defer nc.Unlock()
 
-	nref := nc.nodes[node.NodeID]
-	if nref == nil {
-		return
-	}
-
-	updatedScore := nc.scoreNode(node)
-	if nref.nodeScore != updatedScore {
+	if nref, ok := nc.nodes[node.NodeID]; ok {

Review comment:
       >  It looks like you're unconditionally removing and re-adding the node?
   
   yep. That may be a small performance issue since it removes and re-add the node even if the resources are not changed. However, the code of checking the changes gets more complicated after the type is changed from `float64` to `[]float64`. I prefer to update node unconditionally to keep code simpler. 




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

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] craigcondit commented on a change in pull request #318: [YUNIKORN-831] node sorting should check other resources of nodes ins…

Posted by GitBox <gi...@apache.org>.
craigcondit commented on a change in pull request #318:
URL: https://github.com/apache/incubator-yunikorn-core/pull/318#discussion_r699350349



##########
File path: pkg/scheduler/objects/node_collection.go
##########
@@ -211,13 +213,7 @@ func (nc *baseNodeCollection) NodeUpdated(node *Node) {
 	nc.Lock()
 	defer nc.Unlock()
 
-	nref := nc.nodes[node.NodeID]
-	if nref == nil {
-		return
-	}
-
-	updatedScore := nc.scoreNode(node)
-	if nref.nodeScore != updatedScore {
+	if nref, ok := nc.nodes[node.NodeID]; ok {

Review comment:
       I don't think this is correct. When NodeUpdated() is called, it might be due to usage values changing, and therefore we need to calculate a new score and update nref.nodeScore. If the score has changed, the node needs to be removed from the BTree (looked up using previous version of nref) and re-added using the updated nref. 




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

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] craigcondit commented on a change in pull request #318: [YUNIKORN-831] node sorting should check other resources of nodes ins…

Posted by GitBox <gi...@apache.org>.
craigcondit commented on a change in pull request #318:
URL: https://github.com/apache/incubator-yunikorn-core/pull/318#discussion_r699350349



##########
File path: pkg/scheduler/objects/node_collection.go
##########
@@ -211,13 +213,7 @@ func (nc *baseNodeCollection) NodeUpdated(node *Node) {
 	nc.Lock()
 	defer nc.Unlock()
 
-	nref := nc.nodes[node.NodeID]
-	if nref == nil {
-		return
-	}
-
-	updatedScore := nc.scoreNode(node)
-	if nref.nodeScore != updatedScore {
+	if nref, ok := nc.nodes[node.NodeID]; ok {

Review comment:
       I don't think this is correct. When NodeUpdated() is called, it might be due to usage values changing, and therefore we need to calculate a new score and update nref.nodeScore. If the score has changed, the node needs to be removed from the BTree (looked up using previous version of nref) and re-added using the updated nref. 

##########
File path: pkg/scheduler/objects/node_collection.go
##########
@@ -211,13 +213,7 @@ func (nc *baseNodeCollection) NodeUpdated(node *Node) {
 	nc.Lock()
 	defer nc.Unlock()
 
-	nref := nc.nodes[node.NodeID]
-	if nref == nil {
-		return
-	}
-
-	updatedScore := nc.scoreNode(node)
-	if nref.nodeScore != updatedScore {
+	if nref, ok := nc.nodes[node.NodeID]; ok {

Review comment:
       Actually, I may be mistaken. It looks like you're unconditionally removing and re-adding the node? If so, that also works.




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

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] craigcondit commented on a change in pull request #318: [YUNIKORN-831] node sorting should check other resources of nodes ins…

Posted by GitBox <gi...@apache.org>.
craigcondit commented on a change in pull request #318:
URL: https://github.com/apache/incubator-yunikorn-core/pull/318#discussion_r699539816



##########
File path: pkg/scheduler/objects/node_collection.go
##########
@@ -211,13 +213,7 @@ func (nc *baseNodeCollection) NodeUpdated(node *Node) {
 	nc.Lock()
 	defer nc.Unlock()
 
-	nref := nc.nodes[node.NodeID]
-	if nref == nil {
-		return
-	}
-
-	updatedScore := nc.scoreNode(node)
-	if nref.nodeScore != updatedScore {
+	if nref, ok := nc.nodes[node.NodeID]; ok {

Review comment:
       Makes sense.




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

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] codecov[bot] commented on pull request #318: [YUNIKORN-831] node sorting should check other resources of nodes ins…

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #318:
URL: https://github.com/apache/incubator-yunikorn-core/pull/318#issuecomment-909058455


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#318](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c01180f) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5a1c19e) will **increase** coverage by `2.65%`.
   > The diff coverage is `65.57%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #318      +/-   ##
   ==========================================
   + Coverage   63.46%   66.12%   +2.65%     
   ==========================================
     Files          60       63       +3     
     Lines        5220     5934     +714     
   ==========================================
   + Hits         3313     3924     +611     
   - Misses       1747     1808      +61     
   - Partials      160      202      +42     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [pkg/metrics/init.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvaW5pdC5nbw==) | `62.96% <ø> (ø)` | |
   | [pkg/scheduler/drf\_preemption\_policy.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9kcmZfcHJlZW1wdGlvbl9wb2xpY3kuZ28=) | `0.00% <0.00%> (ø)` | |
   | [pkg/scheduler/nodes\_usage\_monitor.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9ub2Rlc191c2FnZV9tb25pdG9yLmdv) | `0.00% <0.00%> (ø)` | |
   | [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
   | [pkg/scheduler/objects/node\_iterator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGVfaXRlcmF0b3IuZ28=) | `100.00% <ø> (ø)` | |
   | [pkg/scheduler/objects/sorters.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3NvcnRlcnMuZ28=) | `100.00% <ø> (ø)` | |
   | [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `32.25% <ø> (+19.35%)` | :arrow_up: |
   | [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `16.10% <23.33%> (+9.84%)` | :arrow_up: |
   | [pkg/metrics/metrics\_collector.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvbWV0cmljc19jb2xsZWN0b3IuZ28=) | `64.51% <45.00%> (-9.40%)` | :arrow_down: |
   | [pkg/metrics/scheduler.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3Mvc2NoZWR1bGVyLmdv) | `58.92% <49.33%> (-2.66%)` | :arrow_down: |
   | ... and [27 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [66cdb3d...c01180f](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


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

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] codecov[bot] commented on pull request #318: [YUNIKORN-831] node sorting should check other resources of nodes ins…

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #318:
URL: https://github.com/apache/incubator-yunikorn-core/pull/318#issuecomment-909058455


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#318](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c01180f) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5a1c19e) will **increase** coverage by `2.65%`.
   > The diff coverage is `65.57%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #318      +/-   ##
   ==========================================
   + Coverage   63.46%   66.12%   +2.65%     
   ==========================================
     Files          60       63       +3     
     Lines        5220     5934     +714     
   ==========================================
   + Hits         3313     3924     +611     
   - Misses       1747     1808      +61     
   - Partials      160      202      +42     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [pkg/metrics/init.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvaW5pdC5nbw==) | `62.96% <ø> (ø)` | |
   | [pkg/scheduler/drf\_preemption\_policy.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9kcmZfcHJlZW1wdGlvbl9wb2xpY3kuZ28=) | `0.00% <0.00%> (ø)` | |
   | [pkg/scheduler/nodes\_usage\_monitor.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9ub2Rlc191c2FnZV9tb25pdG9yLmdv) | `0.00% <0.00%> (ø)` | |
   | [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
   | [pkg/scheduler/objects/node\_iterator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGVfaXRlcmF0b3IuZ28=) | `100.00% <ø> (ø)` | |
   | [pkg/scheduler/objects/sorters.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3NvcnRlcnMuZ28=) | `100.00% <ø> (ø)` | |
   | [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `32.25% <ø> (+19.35%)` | :arrow_up: |
   | [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `16.10% <23.33%> (+9.84%)` | :arrow_up: |
   | [pkg/metrics/metrics\_collector.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvbWV0cmljc19jb2xsZWN0b3IuZ28=) | `64.51% <45.00%> (-9.40%)` | :arrow_down: |
   | [pkg/metrics/scheduler.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3Mvc2NoZWR1bGVyLmdv) | `58.92% <49.33%> (-2.66%)` | :arrow_down: |
   | ... and [27 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [66cdb3d...c01180f](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/318?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


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

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] chia7712 commented on a change in pull request #318: [YUNIKORN-831] node sorting should check other resources of nodes ins…

Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #318:
URL: https://github.com/apache/incubator-yunikorn-core/pull/318#discussion_r699402769



##########
File path: pkg/scheduler/objects/node_collection.go
##########
@@ -211,13 +213,7 @@ func (nc *baseNodeCollection) NodeUpdated(node *Node) {
 	nc.Lock()
 	defer nc.Unlock()
 
-	nref := nc.nodes[node.NodeID]
-	if nref == nil {
-		return
-	}
-
-	updatedScore := nc.scoreNode(node)
-	if nref.nodeScore != updatedScore {
+	if nref, ok := nc.nodes[node.NodeID]; ok {

Review comment:
       >  It looks like you're unconditionally removing and re-adding the node?
   
   yep. That may be a small performance issue since it removes and re-add the node even if the resources are not changed. However, the code of checking the changes gets more complicated after the type is changed from `float64` to `[]float64`. I prefer to update node unconditionally to keep code simpler. 




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

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] craigcondit commented on a change in pull request #318: [YUNIKORN-831] node sorting should check other resources of nodes ins…

Posted by GitBox <gi...@apache.org>.
craigcondit commented on a change in pull request #318:
URL: https://github.com/apache/incubator-yunikorn-core/pull/318#discussion_r699350349



##########
File path: pkg/scheduler/objects/node_collection.go
##########
@@ -211,13 +213,7 @@ func (nc *baseNodeCollection) NodeUpdated(node *Node) {
 	nc.Lock()
 	defer nc.Unlock()
 
-	nref := nc.nodes[node.NodeID]
-	if nref == nil {
-		return
-	}
-
-	updatedScore := nc.scoreNode(node)
-	if nref.nodeScore != updatedScore {
+	if nref, ok := nc.nodes[node.NodeID]; ok {

Review comment:
       I don't think this is correct. When NodeUpdated() is called, it might be due to usage values changing, and therefore we need to calculate a new score and update nref.nodeScore. If the score has changed, the node needs to be removed from the BTree (looked up using previous version of nref) and re-added using the updated nref. 

##########
File path: pkg/scheduler/objects/node_collection.go
##########
@@ -211,13 +213,7 @@ func (nc *baseNodeCollection) NodeUpdated(node *Node) {
 	nc.Lock()
 	defer nc.Unlock()
 
-	nref := nc.nodes[node.NodeID]
-	if nref == nil {
-		return
-	}
-
-	updatedScore := nc.scoreNode(node)
-	if nref.nodeScore != updatedScore {
+	if nref, ok := nc.nodes[node.NodeID]; ok {

Review comment:
       Actually, I may be mistaken. It looks like you're unconditionally removing and re-adding the node? If so, that also works.

##########
File path: pkg/scheduler/objects/node_collection.go
##########
@@ -211,13 +213,7 @@ func (nc *baseNodeCollection) NodeUpdated(node *Node) {
 	nc.Lock()
 	defer nc.Unlock()
 
-	nref := nc.nodes[node.NodeID]
-	if nref == nil {
-		return
-	}
-
-	updatedScore := nc.scoreNode(node)
-	if nref.nodeScore != updatedScore {
+	if nref, ok := nc.nodes[node.NodeID]; ok {

Review comment:
       Makes sense.




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

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-yunikorn-core] chia7712 commented on pull request #318: [YUNIKORN-831] node sorting should check other resources of nodes instead of comparing node id directly

Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #318:
URL: https://github.com/apache/incubator-yunikorn-core/pull/318#issuecomment-920562466


   close as duplicate


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

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org