You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by "pbacsko (via GitHub)" <gi...@apache.org> on 2023/05/05 08:50:19 UTC
[GitHub] [yunikorn-k8shim] pbacsko opened a new pull request, #586: [YUNIKORN-1723] Remove string comparison from hasReadyCondition() function
pbacsko opened a new pull request, #586:
URL: https://github.com/apache/yunikorn-k8shim/pull/586
### What is this PR for?
Remove the comparison `NodeStatus.String()` against `nil`. It's unnecessary and expensive.
### 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-1723
### 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] [yunikorn-k8shim] wilfred-s commented on a diff in pull request #586: [YUNIKORN-1723] Remove string comparison from hasReadyCondition() function
Posted by "wilfred-s (via GitHub)" <gi...@apache.org>.
wilfred-s commented on code in PR #586:
URL: https://github.com/apache/yunikorn-k8shim/pull/586#discussion_r1186986458
##########
pkg/cache/nodes.go:
##########
@@ -217,7 +217,7 @@ func (nc *schedulerNodes) schedulerNodeEventHandler() func(obj interface{}) {
}
func hasReadyCondition(node *v1.Node) bool {
- if node != nil && node.Status.String() != "nil" {
+ if node != nil {
Review Comment:
No it cannot be nil as it is not a pointer it is a NodeStatus struct.
Simple code to demonstrate this works and we do not get a panic:
```
n := v1.Node{}
if len(n.Status.Conditions) == 0 {
fmt.Println(n.Status.String())
}
```
--
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] [yunikorn-k8shim] codecov[bot] commented on pull request #586: [YUNIKORN-1723] Remove string comparison from hasReadyCondition() function
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #586:
URL: https://github.com/apache/yunikorn-k8shim/pull/586#issuecomment-1535955965
## [Codecov](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/586?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 [#586](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/586?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ac4ed5e) into [master](https://app.codecov.io/gh/apache/yunikorn-k8shim/commit/523b6f023d8da78b8b5f2d604efe605a130cbaba?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (523b6f0) will **increase** coverage by `0.02%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #586 +/- ##
==========================================
+ Coverage 69.97% 69.99% +0.02%
==========================================
Files 47 47
Lines 7943 7943
==========================================
+ Hits 5558 5560 +2
+ Misses 2177 2175 -2
Partials 208 208
```
| [Impacted Files](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/586?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/nodes.go](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/586?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) | `81.81% <100.00%> (ø)` | |
... and [1 file with indirect coverage changes](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/586/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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] [yunikorn-k8shim] manirajv06 commented on pull request #586: [YUNIKORN-1723] Remove string comparison from hasReadyCondition() function
Posted by "manirajv06 (via GitHub)" <gi...@apache.org>.
manirajv06 commented on PR #586:
URL: https://github.com/apache/yunikorn-k8shim/pull/586#issuecomment-1537699169
looks good.
--
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] [yunikorn-k8shim] wilfred-s commented on a diff in pull request #586: [YUNIKORN-1723] Remove string comparison from hasReadyCondition() function
Posted by "wilfred-s (via GitHub)" <gi...@apache.org>.
wilfred-s commented on code in PR #586:
URL: https://github.com/apache/yunikorn-k8shim/pull/586#discussion_r1186986458
##########
pkg/cache/nodes.go:
##########
@@ -217,7 +217,7 @@ func (nc *schedulerNodes) schedulerNodeEventHandler() func(obj interface{}) {
}
func hasReadyCondition(node *v1.Node) bool {
- if node != nil && node.Status.String() != "nil" {
+ if node != nil {
Review Comment:
No it cannot be nil as it is not a pointer it is a NodeStatus struct.
--
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] [yunikorn-k8shim] yangwwei commented on a diff in pull request #586: [YUNIKORN-1723] Remove string comparison from hasReadyCondition() function
Posted by "yangwwei (via GitHub)" <gi...@apache.org>.
yangwwei commented on code in PR #586:
URL: https://github.com/apache/yunikorn-k8shim/pull/586#discussion_r1186615602
##########
pkg/cache/nodes.go:
##########
@@ -217,7 +217,7 @@ func (nc *schedulerNodes) schedulerNodeEventHandler() func(obj interface{}) {
}
func hasReadyCondition(node *v1.Node) bool {
- if node != nil && node.Status.String() != "nil" {
+ if node != nil {
Review Comment:
can node.Status be a nil? do we need
```
node.Status != nil?
```
--
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] [yunikorn-k8shim] pbacsko closed pull request #586: [YUNIKORN-1723] Remove string comparison from hasReadyCondition() function
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko closed pull request #586: [YUNIKORN-1723] Remove string comparison from hasReadyCondition() function
URL: https://github.com/apache/yunikorn-k8shim/pull/586
--
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