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 2022/03/09 10:29:27 UTC
[GitHub] [incubator-yunikorn-k8shim] manirajv06 opened a new pull request #380: [YUNIKORN-1090] remove common.Node from the shim
manirajv06 opened a new pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380
### What is this PR for?
Enhanced si_helper methods to create/update/delete node request and use these methods directly instead of using common.Node method wrappers to avoid the unnecessary object creations. Also re-used the methods in other place as well.
### What type of PR is it?
* [ ] - Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-1090
### 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-k8shim] pbacsko commented on pull request #380: [YUNIKORN-1090] remove common.Node from the shim
Posted by GitBox <gi...@apache.org>.
pbacsko commented on pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380#issuecomment-1062981601
Mostly LGTM, I added two minor comments.
--
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-k8shim] pbacsko commented on a change in pull request #380: [YUNIKORN-1090] remove common.Node from the shim
Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380#discussion_r822706725
##########
File path: pkg/cache/nodes.go
##########
@@ -213,21 +212,22 @@ func (nc *schedulerNodes) updateNode(oldNode, newNode *v1.Node) {
return
}
- if schedulerNode, ok := nc.nodesMap[newNode.Name]; ok {
- schedulerNode.ready = common.HasReadyCondition(newNode)
+ schedulerNode, ok := nc.nodesMap[newNode.Name]
+ if ok {
+ schedulerNode.ready = hasReadyCondition(newNode)
Review comment:
The logic here changed slightly. We only send out the update to the proxy if the node is found in the `nodesMap`. I don't think it has substantial effects, just wondering if this is better?
cc @wilfred-s
--
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-k8shim] codecov[bot] edited a comment on pull request #380: [YUNIKORN-1090] remove common.Node from the shim
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380#issuecomment-1063040296
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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 [#380](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (80acbad) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/a2f04d18e7342490e6027ee04f9c3d3b2658839f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a2f04d1) will **increase** coverage by `0.77%`.
> The diff coverage is `96.10%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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 #380 +/- ##
==========================================
+ Coverage 64.86% 65.64% +0.77%
==========================================
Files 41 40 -1
Lines 6268 6212 -56
==========================================
+ Hits 4066 4078 +12
+ Misses 2046 1979 -67
+ Partials 156 155 -1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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/cache/node.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL25vZGUuZ28=) | `90.13% <85.71%> (-1.54%)` | :arrow_down: |
| [pkg/cache/nodes.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL25vZGVzLmdv) | `78.98% <100.00%> (+1.82%)` | :arrow_up: |
| [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `99.34% <100.00%> (+36.35%)` | :arrow_up: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `71.74% <0.00%> (-0.06%)` | :arrow_down: |
| [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `43.47% <0.00%> (+0.15%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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-k8shim/pull/380?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 [a2f04d1...80acbad](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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-k8shim] codecov[bot] edited a comment on pull request #380: [YUNIKORN-1090] remove common.Node from the shim
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380#issuecomment-1063040296
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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 [#380](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8ed68f8) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/a2f04d18e7342490e6027ee04f9c3d3b2658839f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a2f04d1) will **increase** coverage by `0.77%`.
> The diff coverage is `96.10%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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 #380 +/- ##
==========================================
+ Coverage 64.86% 65.64% +0.77%
==========================================
Files 41 40 -1
Lines 6268 6212 -56
==========================================
+ Hits 4066 4078 +12
+ Misses 2046 1979 -67
+ Partials 156 155 -1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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/cache/node.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL25vZGUuZ28=) | `90.13% <85.71%> (-1.54%)` | :arrow_down: |
| [pkg/cache/nodes.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL25vZGVzLmdv) | `78.98% <100.00%> (+1.82%)` | :arrow_up: |
| [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `99.34% <100.00%> (+36.35%)` | :arrow_up: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `71.74% <0.00%> (-0.06%)` | :arrow_down: |
| [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `43.47% <0.00%> (+0.15%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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-k8shim/pull/380?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 [a2f04d1...8ed68f8](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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-k8shim] codecov[bot] edited a comment on pull request #380: [YUNIKORN-1090] remove common.Node from the shim
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380#issuecomment-1063040296
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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 [#380](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (663ec57) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/a2f04d18e7342490e6027ee04f9c3d3b2658839f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a2f04d1) will **increase** coverage by `0.63%`.
> The diff coverage is `76.74%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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 #380 +/- ##
==========================================
+ Coverage 64.86% 65.50% +0.63%
==========================================
Files 41 40 -1
Lines 6268 6210 -58
==========================================
+ Hits 4066 4068 +2
+ Misses 2046 1986 -60
Partials 156 156
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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/cache/node.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL25vZGUuZ28=) | `89.13% <57.14%> (-2.54%)` | :arrow_down: |
| [pkg/cache/nodes.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL25vZGVzLmdv) | `75.44% <61.11%> (-1.72%)` | :arrow_down: |
| [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `99.35% <100.00%> (+36.36%)` | :arrow_up: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `71.74% <0.00%> (-0.06%)` | :arrow_down: |
| [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `43.47% <0.00%> (+0.15%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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-k8shim/pull/380?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 [a2f04d1...663ec57](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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-k8shim] manirajv06 commented on a change in pull request #380: [YUNIKORN-1090] remove common.Node from the shim
Posted by GitBox <gi...@apache.org>.
manirajv06 commented on a change in pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380#discussion_r824543728
##########
File path: pkg/cache/nodes.go
##########
@@ -213,21 +212,22 @@ func (nc *schedulerNodes) updateNode(oldNode, newNode *v1.Node) {
return
Review comment:
Taken care here itself even though YUNIKORN-1091 has been filed for the same
--
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-k8shim] manirajv06 commented on a change in pull request #380: [YUNIKORN-1090] remove common.Node from the shim
Posted by GitBox <gi...@apache.org>.
manirajv06 commented on a change in pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380#discussion_r823379317
##########
File path: pkg/common/si_helper.go
##########
@@ -177,14 +184,11 @@ func CreateUpdateRequestForUpdatedNode(node Node) si.NodeRequest {
return request
}
-func CreateUpdateRequestForDeleteNode(node Node) si.NodeRequest {
+func CreateUpdateRequestForDeleteNode(nodeID string, action si.NodeInfo_ActionFromRM) si.NodeRequest {
Review comment:
"delete" as generic term for drain or decommission, but it does restore as well. hence named it as DeleteOrRestore. Hope this is ok.
--
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-k8shim] manirajv06 edited a comment on pull request #380: [YUNIKORN-1090] remove common.Node from the shim
Posted by GitBox <gi...@apache.org>.
manirajv06 edited a comment on pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380#issuecomment-1064957246
Observed few more things while doing the clean up:
1. I can see few more places where SchedulerNode properties gets updated without having proper lock mechanism in place. For example, `schedulerNode.occupied = common.Add(schedulerNode.occupied, resource)` in `updateNodeOccupiedResources` method. We should call `setOccupiedResource` here.
2. I think we should start exploring and see if `sync.Map` makes sense instead of using `map` given the no. of reads and writes it is going through.
Please share your views.
--
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-k8shim] manirajv06 edited a comment on pull request #380: [YUNIKORN-1090] remove common.Node from the shim
Posted by GitBox <gi...@apache.org>.
manirajv06 edited a comment on pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380#issuecomment-1064957246
Observed few more things while doing the clean up:
1. I can see few more places where SchedulerNode properties gets updated without having proper lock mechanism in place. For example, `schedulerNode.occupied = common.Add(schedulerNode.occupied, resource)` in `updateNodeOccupiedResources` method. We should call `setOccupiedResource` here.
2. I think we should start exploring and see if `sync.Map` makes sense instead of using `map` given the no. of reads and writes it is going through.
3. Among multiple conditions discussed in https://kubernetes.io/docs/concepts/architecture/nodes/#condition, only "Ready" has been used. Should we use other conditions as well to determine a generic `isNodeHealthy` factor and eventually passing to core rather than depending on only one condition?
Please share your views.
--
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-k8shim] wilfred-s closed pull request #380: [YUNIKORN-1090] remove common.Node from the shim
Posted by GitBox <gi...@apache.org>.
wilfred-s closed pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380
--
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-k8shim] codecov[bot] edited a comment on pull request #380: [YUNIKORN-1090] remove common.Node from the shim
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380#issuecomment-1063040296
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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 [#380](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d8b5eac) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/a2f04d18e7342490e6027ee04f9c3d3b2658839f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a2f04d1) will **increase** coverage by `0.63%`.
> The diff coverage is `76.74%`.
> :exclamation: Current head d8b5eac differs from pull request most recent head 663ec57. Consider uploading reports for the commit 663ec57 to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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 #380 +/- ##
==========================================
+ Coverage 64.86% 65.50% +0.63%
==========================================
Files 41 40 -1
Lines 6268 6210 -58
==========================================
+ Hits 4066 4068 +2
+ Misses 2046 1986 -60
Partials 156 156
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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/cache/node.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL25vZGUuZ28=) | `89.13% <57.14%> (-2.54%)` | :arrow_down: |
| [pkg/cache/nodes.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL25vZGVzLmdv) | `75.44% <61.11%> (-1.72%)` | :arrow_down: |
| [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `99.35% <100.00%> (+36.36%)` | :arrow_up: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `71.74% <0.00%> (-0.06%)` | :arrow_down: |
| [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `43.47% <0.00%> (+0.15%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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-k8shim/pull/380?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 [a2f04d1...663ec57](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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-k8shim] wilfred-s commented on a change in pull request #380: [YUNIKORN-1090] remove common.Node from the shim
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380#discussion_r823430214
##########
File path: pkg/cache/nodes.go
##########
@@ -213,21 +212,22 @@ func (nc *schedulerNodes) updateNode(oldNode, newNode *v1.Node) {
return
}
- if schedulerNode, ok := nc.nodesMap[newNode.Name]; ok {
- schedulerNode.ready = common.HasReadyCondition(newNode)
+ schedulerNode, ok := nc.nodesMap[newNode.Name]
+ if ok {
+ schedulerNode.ready = hasReadyCondition(newNode)
if !schedulerNode.ready {
log.Logger().Debug("Node is not ready", zap.String("Node name", newNode.Name))
}
Review comment:
we can log the state of the node always
##########
File path: pkg/cache/nodes.go
##########
@@ -213,21 +212,22 @@ func (nc *schedulerNodes) updateNode(oldNode, newNode *v1.Node) {
return
Review comment:
we must update the resources in the cachedNode, might need locking and a setter method:
`cachedNode.capacity = common.GetNodeResource(&newNode.Status)`
##########
File path: pkg/common/si_helper.go
##########
@@ -133,18 +134,23 @@ func CreateReleaseAllocationRequestForTask(appID, allocUUID, partition, terminat
return result
}
-func CreateUpdateRequestForNewNode(node Node) si.NodeRequest {
+func CreateUpdateRequestForNewNode(nodeID string, capacity *si.Resource, occupied *si.Resource,
Review comment:
Add comment to the exported function:
```
// CreateUpdateRequestForNewNode builds a NodeRequest ....
```
Should do the same for all si_helper functions changed here.
##########
File path: pkg/common/si_helper.go
##########
@@ -133,18 +134,23 @@ func CreateReleaseAllocationRequestForTask(appID, allocUUID, partition, terminat
return result
}
-func CreateUpdateRequestForNewNode(node Node) si.NodeRequest {
+func CreateUpdateRequestForNewNode(nodeID string, capacity *si.Resource, occupied *si.Resource,
+ existingAllocations []*si.Allocation, labels string, ready bool) si.NodeRequest {
// Use node's name as the NodeID, this is because when bind pod to node,
// name of node is required but uid is optional.
nodeInfo := &si.NodeInfo{
- NodeID: node.name,
- SchedulableResource: node.capacity,
+ NodeID: nodeID,
+ SchedulableResource: capacity,
+ OccupiedResource: occupied,
// TODO is this required?
Review comment:
Please remove this TODO
##########
File path: pkg/cache/nodes.go
##########
@@ -213,21 +212,22 @@ func (nc *schedulerNodes) updateNode(oldNode, newNode *v1.Node) {
return
}
- if schedulerNode, ok := nc.nodesMap[newNode.Name]; ok {
- schedulerNode.ready = common.HasReadyCondition(newNode)
+ schedulerNode, ok := nc.nodesMap[newNode.Name]
+ if ok {
+ schedulerNode.ready = hasReadyCondition(newNode)
if !schedulerNode.ready {
log.Logger().Debug("Node is not ready", zap.String("Node name", newNode.Name))
}
+ request := common.CreateUpdateRequestForUpdatedNode(newNode.Name, common.GetNodeResource(&newNode.Status),
+ schedulerNode.occupied, schedulerNode.ready)
+ log.Logger().Info("report updated nodes to scheduler", zap.Any("request", request))
+ if err := nc.proxy.UpdateNode(&request); err != nil {
+ log.Logger().Info("hitting error while handling UpdateNode", zap.Error(err))
+ }
} else {
log.Logger().Error("Unable to find scheduler node in nodes map", zap.String("node name",
Review comment:
can be removed node is always found
##########
File path: pkg/cache/nodes.go
##########
@@ -213,21 +212,22 @@ func (nc *schedulerNodes) updateNode(oldNode, newNode *v1.Node) {
return
}
- if schedulerNode, ok := nc.nodesMap[newNode.Name]; ok {
- schedulerNode.ready = common.HasReadyCondition(newNode)
+ schedulerNode, ok := nc.nodesMap[newNode.Name]
+ if ok {
+ schedulerNode.ready = hasReadyCondition(newNode)
Review comment:
I do think we need to carefully look at what we do here (referencing new line numbers):
* line 195: we get the node out of the nodesMap store in cachedNode
* line 215: we retrieve the same node again, we already know it CANNOT be nil and ok is `true, we should re-use cachedNode
* line 217: should always be executed, even if resources do not change, should this use a locked setter method?
##########
File path: pkg/common/si_helper_test.go
##########
@@ -18,14 +18,21 @@
package common
import (
+ "strconv"
"testing"
+ "github.com/apache/incubator-yunikorn-k8shim/pkg/common/constants"
"github.com/apache/incubator-yunikorn-scheduler-interface/lib/go/common"
+ "github.com/apache/incubator-yunikorn-scheduler-interface/lib/go/si"
+
"gotest.tools/assert"
+
v1 "k8s.io/api/core/v1"
apis "k8s.io/apimachinery/pkg/apis/meta/v1"
Review comment:
Can we re-order as we are cleaning up, 3 groups separated by an empty line:
- group system imports
- group other imports
- group yunikorn imports
##########
File path: pkg/cache/nodes.go
##########
@@ -213,21 +212,22 @@ func (nc *schedulerNodes) updateNode(oldNode, newNode *v1.Node) {
return
}
- if schedulerNode, ok := nc.nodesMap[newNode.Name]; ok {
- schedulerNode.ready = common.HasReadyCondition(newNode)
+ schedulerNode, ok := nc.nodesMap[newNode.Name]
+ if ok {
+ schedulerNode.ready = hasReadyCondition(newNode)
if !schedulerNode.ready {
log.Logger().Debug("Node is not ready", zap.String("Node name", newNode.Name))
}
+ request := common.CreateUpdateRequestForUpdatedNode(newNode.Name, common.GetNodeResource(&newNode.Status),
Review comment:
should use the cachedNode, does this event need to be generated when ready changes without resource changes?
##########
File path: pkg/common/si_helper.go
##########
@@ -156,15 +162,16 @@ func CreateUpdateRequestForNewNode(node Node) si.NodeRequest {
return request
}
-func CreateUpdateRequestForUpdatedNode(node Node) si.NodeRequest {
+func CreateUpdateRequestForUpdatedNode(nodeID string, capacity *si.Resource, occupied *si.Resource,
+ ready bool) si.NodeRequest {
// Currently only includes resource in the update request
Review comment:
Please update this comment as it is incorrect
##########
File path: pkg/cache/nodes.go
##########
@@ -259,3 +258,12 @@ func (nc *schedulerNodes) schedulerNodeEventHandler() func(obj interface{}) {
}
}
}
+
+func hasReadyCondition(node *v1.Node) bool {
+ for _, condition := range node.Status.Conditions {
Review comment:
Can `node` or `Status` be nil? Rather be careful than rely on the externally defined object always being complete.
--
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-k8shim] codecov[bot] edited a comment on pull request #380: [YUNIKORN-1090] remove common.Node from the shim
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380#issuecomment-1063040296
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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 [#380](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (663ec57) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/a2f04d18e7342490e6027ee04f9c3d3b2658839f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a2f04d1) will **increase** coverage by `0.63%`.
> The diff coverage is `76.74%`.
> :exclamation: Current head 663ec57 differs from pull request most recent head 80acbad. Consider uploading reports for the commit 80acbad to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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 #380 +/- ##
==========================================
+ Coverage 64.86% 65.50% +0.63%
==========================================
Files 41 40 -1
Lines 6268 6210 -58
==========================================
+ Hits 4066 4068 +2
+ Misses 2046 1986 -60
Partials 156 156
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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/cache/node.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL25vZGUuZ28=) | `89.13% <57.14%> (-2.54%)` | :arrow_down: |
| [pkg/cache/nodes.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL25vZGVzLmdv) | `75.44% <61.11%> (-1.72%)` | :arrow_down: |
| [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `99.35% <100.00%> (+36.36%)` | :arrow_up: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `71.74% <0.00%> (-0.06%)` | :arrow_down: |
| [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `43.47% <0.00%> (+0.15%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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-k8shim/pull/380?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 [a2f04d1...80acbad](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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-k8shim] manirajv06 commented on pull request #380: [YUNIKORN-1090] remove common.Node from the shim
Posted by GitBox <gi...@apache.org>.
manirajv06 commented on pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380#issuecomment-1064957246
Observed few more things while doing this clean up:
1. I can see few more places where SchedulerNode properties gets updates without having lock mechanism in place. For example, `schedulerNode.occupied = common.Add(schedulerNode.occupied, resource)` in `updateNodeOccupiedResources` method
2. I think we should start exploring and see if `sync.Map` makes sense instead of using `map` given the no. of reads and writes it is going through.
Please share your views.
--
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-k8shim] wilfred-s commented on pull request #380: [YUNIKORN-1090] remove common.Node from the shim
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380#issuecomment-1066328969
> Observed few more things while doing the clean up:
>
> 1. I can see few more places where SchedulerNode properties gets updated without having proper lock mechanism in place. For example, `schedulerNode.occupied = common.Add(schedulerNode.occupied, resource)` in `updateNodeOccupiedResources` method. We should call `setOccupiedResource` here.
We should look at all those and we might need to get this fixed throughout. A follow up jira would be the right approach here.
> 2. I think we should start exploring and see if `sync.Map` makes sense instead of using `map` given the no. of reads and writes it is going through.
The number of writes is minimal. Node updates should not happen at a high rate, not even in large clusters. Reads do occur more often. Optimising the performance of this access would most likely not affect the overall performance. `sync.Map` also has a different behaviour when ranging over entries which could cause small but really difficult to track down bugs.
> 3. Among multiple conditions discussed in https://kubernetes.io/docs/concepts/architecture/nodes/#condition, only "Ready" has been used. Should we use other conditions as well to determine a generic `isNodeHealthy` factor and eventually passing to core rather than depending on only one condition?
That is surely worth considering. We should file a follow up jira and assess what we can do in this area.
--
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-k8shim] pbacsko commented on a change in pull request #380: [YUNIKORN-1090] remove common.Node from the shim
Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380#discussion_r822708232
##########
File path: pkg/common/si_helper.go
##########
@@ -177,14 +184,11 @@ func CreateUpdateRequestForUpdatedNode(node Node) si.NodeRequest {
return request
}
-func CreateUpdateRequestForDeleteNode(node Node) si.NodeRequest {
+func CreateUpdateRequestForDeleteNode(nodeID string, action si.NodeInfo_ActionFromRM) si.NodeRequest {
Review comment:
Nit: the function implies that this called for "delete" action, but we can pass any kind of action. What about creating two functions like `CreateUpdateRequestForDeleteNode()` and `CreateUpdateRequestForDrainNode()`? I think it would be cleaner.
--
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-k8shim] codecov[bot] commented on pull request #380: [YUNIKORN-1090] remove common.Node from the shim
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380#issuecomment-1063040296
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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 [#380](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d8b5eac) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/a2f04d18e7342490e6027ee04f9c3d3b2658839f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a2f04d1) will **increase** coverage by `0.63%`.
> The diff coverage is `76.74%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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 #380 +/- ##
==========================================
+ Coverage 64.86% 65.50% +0.63%
==========================================
Files 41 40 -1
Lines 6268 6210 -58
==========================================
+ Hits 4066 4068 +2
+ Misses 2046 1986 -60
Partials 156 156
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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/cache/node.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL25vZGUuZ28=) | `89.13% <57.14%> (-2.54%)` | :arrow_down: |
| [pkg/cache/nodes.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL25vZGVzLmdv) | `75.44% <61.11%> (-1.72%)` | :arrow_down: |
| [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `99.35% <100.00%> (+36.36%)` | :arrow_up: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `71.74% <0.00%> (-0.06%)` | :arrow_down: |
| [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `43.47% <0.00%> (+0.15%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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-k8shim/pull/380?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 [a2f04d1...d8b5eac](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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-k8shim] manirajv06 commented on a change in pull request #380: [YUNIKORN-1090] remove common.Node from the shim
Posted by GitBox <gi...@apache.org>.
manirajv06 commented on a change in pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380#discussion_r823380068
##########
File path: pkg/cache/nodes.go
##########
@@ -213,21 +212,22 @@ func (nc *schedulerNodes) updateNode(oldNode, newNode *v1.Node) {
return
}
- if schedulerNode, ok := nc.nodesMap[newNode.Name]; ok {
- schedulerNode.ready = common.HasReadyCondition(newNode)
+ schedulerNode, ok := nc.nodesMap[newNode.Name]
+ if ok {
+ schedulerNode.ready = hasReadyCondition(newNode)
Review comment:
Yes, it should not have any effects. I don't see a situation wherein entry doesn't exist in `nodesMap `during "update"
--
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-k8shim] codecov[bot] edited a comment on pull request #380: [YUNIKORN-1090] remove common.Node from the shim
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #380:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/380#issuecomment-1063040296
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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 [#380](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (80acbad) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/a2f04d18e7342490e6027ee04f9c3d3b2658839f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a2f04d1) will **increase** coverage by `0.77%`.
> The diff coverage is `96.10%`.
> :exclamation: Current head 80acbad differs from pull request most recent head 8ed68f8. Consider uploading reports for the commit 8ed68f8 to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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 #380 +/- ##
==========================================
+ Coverage 64.86% 65.64% +0.77%
==========================================
Files 41 40 -1
Lines 6268 6212 -56
==========================================
+ Hits 4066 4078 +12
+ Misses 2046 1979 -67
+ Partials 156 155 -1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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/cache/node.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL25vZGUuZ28=) | `90.13% <85.71%> (-1.54%)` | :arrow_down: |
| [pkg/cache/nodes.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL25vZGVzLmdv) | `78.98% <100.00%> (+1.82%)` | :arrow_up: |
| [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `99.34% <100.00%> (+36.35%)` | :arrow_up: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `71.74% <0.00%> (-0.06%)` | :arrow_down: |
| [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380/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-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `43.47% <0.00%> (+0.15%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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-k8shim/pull/380?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 [a2f04d1...8ed68f8](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/380?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