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 2020/09/01 06:09:44 UTC
[GitHub] [incubator-yunikorn-core] wilfred-s opened a new pull request #201: [YUNIKORN-390] Panic in placement rule evaluation
wilfred-s opened a new pull request #201:
URL: https://github.com/apache/incubator-yunikorn-core/pull/201
Placement rules can specify an arbitrary parent rule and a create flag for
the parent rule. If the parent rule is configured with the create flag set
to "true" and returns a non-existing queue the scheduler panics.
The fact that the parent queue does not exist means it cannot be a leaf
queue and the isLeafQueue() check should be skipped.
Adding tests to cover the parent rule returns to all rules.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] wilfred-s closed pull request #201: [YUNIKORN-390] Panic in placement rule evaluation
Posted by GitBox <gi...@apache.org>.
wilfred-s closed pull request #201:
URL: https://github.com/apache/incubator-yunikorn-core/pull/201
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] TravisBuddy commented on pull request #201: [YUNIKORN-390] Panic in placement rule evaluation
Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #201:
URL: https://github.com/apache/incubator-yunikorn-core/pull/201#issuecomment-684734531
Hey @wilfred-s,
Your changes look good to me!
<a href="https://travis-ci.org/apache/incubator-yunikorn-core/builds/723021347">View build log</a>
###### TravisBuddy Request Identifier: c9980ec0-ec3c-11ea-831e-55c1b07d6d51
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] adamantal commented on a change in pull request #201: [YUNIKORN-390] Panic in placement rule evaluation
Posted by GitBox <gi...@apache.org>.
adamantal commented on a change in pull request #201:
URL: https://github.com/apache/incubator-yunikorn-core/pull/201#discussion_r481027661
##########
File path: pkg/scheduler/placement/tag_rule_test.go
##########
@@ -144,3 +144,112 @@ partitions:
t.Errorf("tag rule with parent queue incorrect queue '%s', error %v", queue, err)
}
}
+
+// Create the structure for the parent rule tests
+// shared by a number of rule tests
+const confParentChild = `
Review comment:
I don't see why it has been put to the tag rule. IDEs provide quick jump to the definition, but it's a bit counterintuitive. I think this can be moved to `rule_test.go` or `test_rule.go`, or even a helper go file can be created for this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] adamantal commented on a change in pull request #201: [YUNIKORN-390] Panic in placement rule evaluation
Posted by GitBox <gi...@apache.org>.
adamantal commented on a change in pull request #201:
URL: https://github.com/apache/incubator-yunikorn-core/pull/201#discussion_r482181739
##########
File path: pkg/scheduler/placement/tag_rule_test.go
##########
@@ -144,3 +144,112 @@ partitions:
t.Errorf("tag rule with parent queue incorrect queue '%s', error %v", queue, err)
}
}
+
+// Create the structure for the parent rule tests
+// shared by a number of rule tests
+const confParentChild = `
Review comment:
Yes, let's keep it here then.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] wilfred-s commented on pull request #201: [YUNIKORN-390] Panic in placement rule evaluation
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on pull request #201:
URL: https://github.com/apache/incubator-yunikorn-core/pull/201#issuecomment-686230188
If there are no more questions outstanding can someone please approve the change so we can merge this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] codecov[bot] commented on pull request #201: [YUNIKORN-390] Panic in placement rule evaluation
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #201:
URL: https://github.com/apache/incubator-yunikorn-core/pull/201#issuecomment-684734683
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/201?src=pr&el=h1) Report
> Merging [#201](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/201?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/57d663e73cb10674873b119ae1eaf61ad43fdcd9?el=desc) will **increase** coverage by `0.26%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/201?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #201 +/- ##
==========================================
+ Coverage 60.61% 60.88% +0.26%
==========================================
Files 67 67
Lines 6533 6537 +4
==========================================
+ Hits 3960 3980 +20
+ Misses 2427 2419 -8
+ Partials 146 138 -8
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/201?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/scheduler/placement/fixed\_rule.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/201/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wbGFjZW1lbnQvZml4ZWRfcnVsZS5nbw==) | `76.00% <100.00%> (+12.73%)` | :arrow_up: |
| [pkg/scheduler/placement/provided\_rule.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/201/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wbGFjZW1lbnQvcHJvdmlkZWRfcnVsZS5nbw==) | `80.00% <100.00%> (+9.54%)` | :arrow_up: |
| [pkg/scheduler/placement/tag\_rule.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/201/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wbGFjZW1lbnQvdGFnX3J1bGUuZ28=) | `80.00% <100.00%> (+4.48%)` | :arrow_up: |
| [pkg/scheduler/placement/user\_rule.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/201/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wbGFjZW1lbnQvdXNlcl9ydWxlLmdv) | `78.57% <100.00%> (+10.27%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/201?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/201?src=pr&el=footer). Last update [57d663e...ba82469](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/201?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] TravisBuddy commented on pull request #201: [YUNIKORN-390] Panic in placement rule evaluation
Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #201:
URL: https://github.com/apache/incubator-yunikorn-core/pull/201#issuecomment-684446112
## Travis tests have failed
Hey @wilfred-s,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.
### 1st Build
<a href="https://travis-ci.org/apache/incubator-yunikorn-core/jobs/722976823">View build log</a>
<details>
<summary>
<strong>
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.22.2
</strong>
</summary>
```
golangci/golangci-lint info checking GitHub for tag 'v1.22.2'
golangci/golangci-lint info found version: 1.22.2 for v1.22.2/linux/amd64
golangci/golangci-lint info installed /home/travis/gopath/bin/golangci-lint
```
</details>
<details>
<summary>
<strong>
make lint
</strong>
</summary>
```
running golangci-lint
pkg/scheduler/placement/fixed_rule_test.go:162:10: string `
partitions:
- name: default
queues:
- name: testchild
- name: testparent
parent: true
` has 4 occurrences, make it a constant (goconst)
data := `
^
pkg/scheduler/placement/fixed_rule_test.go:236:14: string `root.testparentnew.testchild` has 4 occurrences, make it a constant (goconst)
if queue != "root.testparentnew.testchild" || err != nil {
^
pkg/scheduler/placement/tag_rule_test.go:160:2: ineffectual assignment to `tags` (ineffassign)
tags := make(map[string]string)
^
Makefile:45: recipe for target 'lint' failed
make: *** [lint] Error 1
```
</details>
###### TravisBuddy Request Identifier: abb9c760-ec1c-11ea-831e-55c1b07d6d51
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] wilfred-s commented on a change in pull request #201: [YUNIKORN-390] Panic in placement rule evaluation
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #201:
URL: https://github.com/apache/incubator-yunikorn-core/pull/201#discussion_r481831765
##########
File path: pkg/scheduler/placement/tag_rule_test.go
##########
@@ -144,3 +144,112 @@ partitions:
t.Errorf("tag rule with parent queue incorrect queue '%s', error %v", queue, err)
}
}
+
+// Create the structure for the parent rule tests
+// shared by a number of rule tests
+const confParentChild = `
Review comment:
We cannot move it to `test_rule.go` as that is a part of the production code (i.e. does not end in `_test.go`) and it really does not belong there.
The `rule_test.go` is testing the interface for the rule object, also not really the place to set it as it is not even referenced there. Probably placing it in the top of one of the test files would have been better, just forgot to do that. Adding a new file for just two constants seems a bit much.
There is no real solution that solves it nicely without the need to jump. It might be better for readability to add it to the `rule_test.go` file. It still requires the jumping to find the value, We can do that a a bit of an in between solution, are you OK with that then?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org