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/10/21 11:28:27 UTC
[GitHub] [incubator-yunikorn-core] kingamarton opened a new pull request #219: Yunikorn 352 - fixed max resource check and behavioural change
kingamarton opened a new pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219
----------------------------------------------------------------
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] edited a comment on pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-713539637
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=h1) Report
> Merging [#219](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=desc) (d638ff3) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/01f5288eb5931a16834e0402af9efb308f946ad4?el=desc) (01f5288) will **decrease** coverage by `2.19%`.
> The diff coverage is `74.24%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
- Coverage 63.12% 60.92% -2.20%
==========================================
Files 59 67 +8
Lines 4879 5815 +936
==========================================
+ Hits 3080 3543 +463
- Misses 1657 2121 +464
- Partials 142 151 +9
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `91.91% <21.05%> (-5.60%)` | :arrow_down: |
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `89.23% <95.74%> (+2.22%)` | :arrow_up: |
| [pkg/common/utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi91dGlscy5nbw==) | `66.66% <0.00%> (-5.21%)` | :arrow_down: |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `53.89% <0.00%> (-1.19%)` | :arrow_down: |
| [pkg/scheduler/preemptor.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wcmVlbXB0b3IuZ28=) | `0.00% <0.00%> (ø)` | |
| [pkg/scheduler/scheduler.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9zY2hlZHVsZXIuZ28=) | `0.00% <0.00%> (ø)` | |
| [pkg/scheduler/node\_iterator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9ub2RlX2l0ZXJhdG9yLmdv) | `100.00% <0.00%> (ø)` | |
| [pkg/scheduler/partition\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb25fbWFuYWdlci5nbw==) | `0.00% <0.00%> (ø)` | |
| ... and [44 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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/219?src=pr&el=footer). Last update [01f5288...572c116](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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] codecov[bot] commented on pull request #219: Yunikorn 352 - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-713539637
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=h1) Report
> Merging [#219](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/e9ab51a96afc6a9ce9a320db800f514cf5e7ab50?el=desc) will **increase** coverage by `0.06%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
+ Coverage 60.90% 60.97% +0.06%
==========================================
Files 67 67
Lines 5778 5788 +10
==========================================
+ Hits 3519 3529 +10
Misses 2114 2114
Partials 145 145
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `87.70% <100.00%> (+0.69%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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/219?src=pr&el=footer). Last update [e9ab51a...fabda8d](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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] codecov[bot] edited a comment on pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-713539637
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=h1) Report
> Merging [#219](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=desc) (572c116) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/01f5288eb5931a16834e0402af9efb308f946ad4?el=desc) (01f5288) will **decrease** coverage by `0.26%`.
> The diff coverage is `57.14%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
- Coverage 63.12% 62.86% -0.27%
==========================================
Files 59 59
Lines 4879 4885 +6
==========================================
- Hits 3080 3071 -9
- Misses 1657 1665 +8
- Partials 142 149 +7
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `92.58% <19.04%> (-4.94%)` | :arrow_down: |
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `87.50% <85.71%> (+0.49%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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/219?src=pr&el=footer). Last update [01f5288...2167ed9](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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 #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-717788304
## Travis tests have failed
Hey @kingamarton,
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/739540283">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/common/resources/resources.go:807: File is not `gofmt`-ed with `-s` (gofmt)
if r == nil {return true}
pkg/common/configs/configvalidator.go:113: File is not `gofmt`-ed with `-s` (gofmt)
if err !=nil {
pkg/common/configs/configvalidator_test.go:45: File is not `gofmt`-ed with `-s` (gofmt)
true,
"invalid guaranteed resource map[memory:-50 vcores:33] for queue , cannot be negative"},
pkg/common/configs/configvalidator.go:101:7: shadow: declaration of "err" shadows declaration at line 80 (govet)
if err := checkResourceConfigurationsForQueue(cur.Queues[i], maxToPass, guaranteedToPass); err != nil {
^
Makefile:45: recipe for target 'lint' failed
make: *** [lint] Error 1
```
</details>
###### TravisBuddy Request Identifier: 6ac220d0-18fa-11eb-86f8-31dd4df69769
----------------------------------------------------------------
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] edited a comment on pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-713539637
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=h1) Report
> Merging [#219](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=desc) (154c30b) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5471d84cd619dc9b6c40e970b0e9d53aa3f8e07f?el=desc) (5471d84) will **decrease** coverage by `0.87%`.
> The diff coverage is `91.48%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
- Coverage 64.02% 63.15% -0.88%
==========================================
Files 60 59 -1
Lines 4984 4882 -102
==========================================
- Hits 3191 3083 -108
- Misses 1636 1657 +21
+ Partials 157 142 -15
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `87.50% <85.71%> (+0.49%)` | :arrow_up: |
| [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `97.60% <100.00%> (+1.58%)` | :arrow_up: |
| [pkg/scheduler/partition\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb25fbWFuYWdlci5nbw==) | `0.00% <0.00%> (-16.00%)` | :arrow_down: |
| [pkg/events/event\_publisher.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2V2ZW50cy9ldmVudF9wdWJsaXNoZXIuZ28=) | `88.88% <0.00%> (-11.12%)` | :arrow_down: |
| [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `0.00% <0.00%> (-6.44%)` | :arrow_down: |
| [pkg/scheduler/partition.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `64.90% <0.00%> (-1.46%)` | :arrow_down: |
| [pkg/metrics/init.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL21ldHJpY3MvaW5pdC5nbw==) | `62.96% <0.00%> (ø)` | |
| [pkg/scheduler/health\_checker.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9oZWFsdGhfY2hlY2tlci5nbw==) | | |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `55.08% <0.00%> (+0.94%)` | :arrow_up: |
| ... and [1 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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/219?src=pr&el=footer). Last update [5471d84...c40bd9d](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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] kingamarton commented on a change in pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#discussion_r542358608
##########
File path: pkg/common/resources/resources.go
##########
@@ -742,6 +743,37 @@ func ComponentWiseMin(left, right *Resource) *Resource {
return out
}
+// Returns a new Resource with the smallest value for each quantity in the Resources
+// If either Resource passed in is nil the other Resource is returned
+// If a Resource type is missing from one of the Resource, it is considered empty and the quantity from the other Resource is returned
+func ComponentWiseMinPermissive(left, right *Resource) *Resource {
+ out := NewResource()
+ if right == nil && left == nil {
+ return out
+ }
Review comment:
You are right! Changed to 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] kingamarton commented on a change in pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#discussion_r542360103
##########
File path: pkg/common/resources/resources.go
##########
@@ -742,6 +743,37 @@ func ComponentWiseMin(left, right *Resource) *Resource {
return out
}
+// Returns a new Resource with the smallest value for each quantity in the Resources
+// If either Resource passed in is nil the other Resource is returned
+// If a Resource type is missing from one of the Resource, it is considered empty and the quantity from the other Resource is returned
+func ComponentWiseMinPermissive(left, right *Resource) *Resource {
+ out := NewResource()
+ if right == nil && left == nil {
+ return out
+ }
+ if left == nil {
+ return right
+ }
+ if right == nil {
+ return left
+ }
Review comment:
Done
----------------------------------------------------------------
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] edited a comment on pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-713539637
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=h1) Report
> Merging [#219](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=desc) (c40bd9d) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5471d84cd619dc9b6c40e970b0e9d53aa3f8e07f?el=desc) (5471d84) will **increase** coverage by `0.07%`.
> The diff coverage is `86.79%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
+ Coverage 64.02% 64.10% +0.07%
==========================================
Files 60 60
Lines 4984 4992 +8
==========================================
+ Hits 3191 3200 +9
- Misses 1636 1638 +2
+ Partials 157 154 -3
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `95.32% <78.26%> (-0.71%)` | :arrow_down: |
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `88.82% <93.33%> (+1.81%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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/219?src=pr&el=footer). Last update [5471d84...c40bd9d](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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 #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-718242895
Hey @kingamarton,
Your changes look good to me!
<a href="https://travis-ci.org/apache/incubator-yunikorn-core/builds/739720491">View build log</a>
###### TravisBuddy Request Identifier: 22e8b310-196c-11eb-a6ec-b9eb403ed3be
----------------------------------------------------------------
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] edited a comment on pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-713539637
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=h1) Report
> Merging [#219](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=desc) (c415088) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5471d84cd619dc9b6c40e970b0e9d53aa3f8e07f?el=desc) (5471d84) will **increase** coverage by `0.21%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
+ Coverage 64.02% 64.24% +0.21%
==========================================
Files 60 60
Lines 4984 4992 +8
==========================================
+ Hits 3191 3207 +16
+ Misses 1636 1632 -4
+ Partials 157 153 -4
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `90.00% <100.00%> (+2.99%)` | :arrow_up: |
| [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `96.78% <100.00%> (+0.75%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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/219?src=pr&el=footer). Last update [5471d84...c415088](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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] codecov[bot] edited a comment on pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-713539637
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=h1) Report
> Merging [#219](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/e9ab51a96afc6a9ce9a320db800f514cf5e7ab50?el=desc) will **increase** coverage by `0.06%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
+ Coverage 60.90% 60.97% +0.06%
==========================================
Files 67 67
Lines 5778 5788 +10
==========================================
+ Hits 3519 3529 +10
Misses 2114 2114
Partials 145 145
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `87.70% <100.00%> (+0.69%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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/219?src=pr&el=footer). Last update [e9ab51a...fa35f6e](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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] codecov[bot] edited a comment on pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-713539637
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=h1) Report
> Merging [#219](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=desc) (2167ed9) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/01f5288eb5931a16834e0402af9efb308f946ad4?el=desc) (01f5288) will **decrease** coverage by `0.18%`.
> The diff coverage is `65.95%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
- Coverage 63.12% 62.94% -0.19%
==========================================
Files 59 59
Lines 4879 4882 +3
==========================================
- Hits 3080 3073 -7
- Misses 1657 1661 +4
- Partials 142 148 +6
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `94.01% <36.84%> (-3.51%)` | :arrow_down: |
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `87.50% <85.71%> (+0.49%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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/219?src=pr&el=footer). Last update [01f5288...154c30b](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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] codecov[bot] edited a comment on pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-713539637
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=h1) Report
> Merging [#219](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/e9ab51a96afc6a9ce9a320db800f514cf5e7ab50?el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `74.24%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
+ Coverage 60.90% 60.92% +0.02%
==========================================
Files 67 67
Lines 5778 5815 +37
==========================================
+ Hits 3519 3543 +24
- Misses 2114 2121 +7
- Partials 145 151 +6
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `91.91% <21.05%> (-4.28%)` | :arrow_down: |
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `89.23% <95.74%> (+2.22%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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/219?src=pr&el=footer). Last update [e9ab51a...d638ff3](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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] codecov[bot] edited a comment on pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-713539637
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=h1) Report
> Merging [#219](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/e9ab51a96afc6a9ce9a320db800f514cf5e7ab50?el=desc) will **increase** coverage by `0.06%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
+ Coverage 60.90% 60.97% +0.06%
==========================================
Files 67 67
Lines 5778 5788 +10
==========================================
+ Hits 3519 3529 +10
Misses 2114 2114
Partials 145 145
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `87.70% <100.00%> (+0.69%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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/219?src=pr&el=footer). Last update [e9ab51a...d638ff3](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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] codecov[bot] edited a comment on pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-713539637
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=h1) Report
> Merging [#219](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=desc) (154c30b) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/01f5288eb5931a16834e0402af9efb308f946ad4?el=desc) (01f5288) will **increase** coverage by `0.02%`.
> The diff coverage is `91.48%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
+ Coverage 63.12% 63.15% +0.02%
==========================================
Files 59 59
Lines 4879 4882 +3
==========================================
+ Hits 3080 3083 +3
Misses 1657 1657
Partials 142 142
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `87.50% <85.71%> (+0.49%)` | :arrow_up: |
| [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `97.60% <100.00%> (+0.08%)` | :arrow_up: |
| [pkg/events/event\_publisher.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2V2ZW50cy9ldmVudF9wdWJsaXNoZXIuZ28=) | `88.88% <0.00%> (-11.12%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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/219?src=pr&el=footer). Last update [01f5288...154c30b](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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] kingamarton commented on a change in pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#discussion_r542347516
##########
File path: pkg/common/resources/resources.go
##########
@@ -771,6 +803,13 @@ func IsZero(zero *Resource) bool {
return true
}
+func IsEmpty(r *Resource) bool {
+ if r == nil {
+ return true
+ }
+ return len(r.Resources) == 0
+}
+
Review comment:
Removed this method and extended isZero check with checking if the resources are empty and if yes handle it as zero resource
----------------------------------------------------------------
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 #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#discussion_r522586803
##########
File path: pkg/common/configs/configvalidator.go
##########
@@ -75,65 +75,100 @@ func checkACL(acl string) error {
// Check the queue resource configuration settings.
// - child or children cannot have higher maximum or guaranteed limits than parents
// - children (added together) cannot have a higher guaranteed setting than a parent
-func checkResourceConfigurationsForQueue(cur *QueueConfig, parent *QueueConfig) error {
+func checkResourceConfigurationsForQueue(cur QueueConfig, parentMax *resources.Resource, parentGuaranteed *resources.Resource) error {
// If cur has children, make sure sum of children's guaranteed <= cur.guaranteed
- curGuaranteedRes, err := resources.NewResourceFromConf(cur.Resources.Guaranteed)
+ actualGuaranteed, err := resources.NewResourceFromConf(cur.Resources.Guaranteed)
if err != nil {
return err
}
- if curGuaranteedRes.HasNegativeValue() {
- return fmt.Errorf("invalid guaranteed resource %v for queue %s, cannot be negative", curGuaranteedRes, cur.Name)
+ if actualGuaranteed.HasNegativeValue() {
+ return fmt.Errorf("invalid guaranteed resource %v for queue %s, cannot be negative", actualGuaranteed, cur.Name)
}
- curMaxRes, err := resources.NewResourceFromConf(cur.Resources.Max)
+ actualMax, err := resources.NewResourceFromConf(cur.Resources.Max)
if err != nil {
return err
}
- if curMaxRes.HasNegativeValue() {
- return fmt.Errorf("invalid max resource %v for queue %s, cannot be negative", curMaxRes, cur.Name)
+ if actualMax.HasNegativeValue() {
+ return fmt.Errorf("invalid max resource %v for queue %s, cannot be negative", actualMax, cur.Name)
}
+ maxToPass := resources.ComponentWiseMinPermissive(actualMax, parentMax)
+ guaranteedToPass := resources.ComponentWiseMinPermissive(maxToPass, actualGuaranteed)
+
if len(cur.Queues) > 0 {
// Check children
for i := range cur.Queues {
- if err := checkResourceConfigurationsForQueue(&cur.Queues[i], cur); err != nil {
+ if err = checkResourceConfigurationsForQueue(cur.Queues[i], maxToPass, guaranteedToPass); err != nil {
return err
}
}
- sum := resources.NewResource()
- for _, child := range cur.Queues {
- childGuaranteed, err := resources.NewResourceFromConf(child.Resources.Guaranteed)
- if err != nil {
- return err
- }
- sum.AddTo(childGuaranteed)
+ err = checkGuaranteedResource(cur.Queues, actualGuaranteed, parentGuaranteed, cur.Name)
+ if err != nil {
+ return err
}
+ }
- if cur.Resources.Guaranteed != nil {
- if !resources.FitIn(curGuaranteedRes, sum) {
- return fmt.Errorf("queue %s has guaranteed-resources (%v) smaller than sum of children guaranteed resources (%v)", cur.Name, curGuaranteedRes, sum)
- }
- } else {
- cur.Resources.Guaranteed = sum.ToConf()
Review comment:
This `ToConf()` was a new function introduced in the previous PR for this issue, (see this [diff](https://github.com/apache/incubator-yunikorn-core/pull/215/files#diff-3c6f56d55b9195cdb71e2693fa0cfce3f28f63a8ff144dcb8e80bcb468c73612R116). It is the only place it was used we now need to revert the add for this function and the tests in the Resource object.
##########
File path: pkg/common/configs/configvalidator.go
##########
@@ -75,65 +75,100 @@ func checkACL(acl string) error {
// Check the queue resource configuration settings.
// - child or children cannot have higher maximum or guaranteed limits than parents
// - children (added together) cannot have a higher guaranteed setting than a parent
-func checkResourceConfigurationsForQueue(cur *QueueConfig, parent *QueueConfig) error {
+func checkResourceConfigurationsForQueue(cur QueueConfig, parentMax *resources.Resource, parentGuaranteed *resources.Resource) error {
// If cur has children, make sure sum of children's guaranteed <= cur.guaranteed
- curGuaranteedRes, err := resources.NewResourceFromConf(cur.Resources.Guaranteed)
+ actualGuaranteed, err := resources.NewResourceFromConf(cur.Resources.Guaranteed)
if err != nil {
return err
}
- if curGuaranteedRes.HasNegativeValue() {
- return fmt.Errorf("invalid guaranteed resource %v for queue %s, cannot be negative", curGuaranteedRes, cur.Name)
+ if actualGuaranteed.HasNegativeValue() {
+ return fmt.Errorf("invalid guaranteed resource %v for queue %s, cannot be negative", actualGuaranteed, cur.Name)
Review comment:
`HasNegativeValue()` is only called after a `NewResourceFromConf()` why have we not folded the check into the creation of the new resource from the config? Having a negative resource in the configuration should immediately be a failure.
We had the split into a config check and later a create of the resource as the processing was split. Now that the processing is one step we should also do the same for the check inside the create.
##########
File path: pkg/common/resources/resources.go
##########
@@ -742,6 +743,37 @@ func ComponentWiseMin(left, right *Resource) *Resource {
return out
}
+// Returns a new Resource with the smallest value for each quantity in the Resources
+// If either Resource passed in is nil the other Resource is returned
+// If a Resource type is missing from one of the Resource, it is considered empty and the quantity from the other Resource is returned
+func ComponentWiseMinPermissive(left, right *Resource) *Resource {
+ out := NewResource()
+ if right == nil && left == nil {
+ return out
+ }
Review comment:
Should this not return a nil value?
if max for parent is nil and the max for parent.child is nil the result should be a nil also. Otherwise we have a zero max as a result.
##########
File path: pkg/common/configs/configvalidator.go
##########
@@ -75,65 +75,100 @@ func checkACL(acl string) error {
// Check the queue resource configuration settings.
// - child or children cannot have higher maximum or guaranteed limits than parents
// - children (added together) cannot have a higher guaranteed setting than a parent
-func checkResourceConfigurationsForQueue(cur *QueueConfig, parent *QueueConfig) error {
+func checkResourceConfigurationsForQueue(cur QueueConfig, parentMax *resources.Resource, parentGuaranteed *resources.Resource) error {
Review comment:
You do not need to repeat the type for the parameter if it is the same:
i.e. `parentMax, parentGuaranteed *resources.Resource`
This should be fixed in multiple places in this change.
##########
File path: pkg/common/resources/resources.go
##########
@@ -771,6 +803,13 @@ func IsZero(zero *Resource) bool {
return true
}
+func IsEmpty(r *Resource) bool {
+ if r == nil {
+ return true
+ }
+ return len(r.Resources) == 0
+}
+
Review comment:
This does nothing more than `IsZero()`. An empty resource as you have defined here is always a zero resource.
This is also only called after a `NewResourceFromConf()` I think we need to fix that one method to do all the checks and in the config validation be able to handle a `nil` resource to be returned in the case that nothing is set.
##########
File path: pkg/common/resources/resources.go
##########
@@ -742,6 +743,37 @@ func ComponentWiseMin(left, right *Resource) *Resource {
return out
}
+// Returns a new Resource with the smallest value for each quantity in the Resources
+// If either Resource passed in is nil the other Resource is returned
+// If a Resource type is missing from one of the Resource, it is considered empty and the quantity from the other Resource is returned
+func ComponentWiseMinPermissive(left, right *Resource) *Resource {
+ out := NewResource()
+ if right == nil && left == nil {
+ return out
+ }
+ if left == nil {
+ return right
+ }
+ if right == nil {
+ return left
+ }
Review comment:
Should we not return a cloned resource in these cases. Returning the same pointer could cause issues..
##########
File path: pkg/common/configs/configvalidator.go
##########
@@ -75,65 +75,100 @@ func checkACL(acl string) error {
// Check the queue resource configuration settings.
// - child or children cannot have higher maximum or guaranteed limits than parents
// - children (added together) cannot have a higher guaranteed setting than a parent
-func checkResourceConfigurationsForQueue(cur *QueueConfig, parent *QueueConfig) error {
+func checkResourceConfigurationsForQueue(cur QueueConfig, parentMax *resources.Resource, parentGuaranteed *resources.Resource) error {
// If cur has children, make sure sum of children's guaranteed <= cur.guaranteed
- curGuaranteedRes, err := resources.NewResourceFromConf(cur.Resources.Guaranteed)
+ actualGuaranteed, err := resources.NewResourceFromConf(cur.Resources.Guaranteed)
if err != nil {
return err
}
- if curGuaranteedRes.HasNegativeValue() {
- return fmt.Errorf("invalid guaranteed resource %v for queue %s, cannot be negative", curGuaranteedRes, cur.Name)
+ if actualGuaranteed.HasNegativeValue() {
+ return fmt.Errorf("invalid guaranteed resource %v for queue %s, cannot be negative", actualGuaranteed, cur.Name)
}
- curMaxRes, err := resources.NewResourceFromConf(cur.Resources.Max)
+ actualMax, err := resources.NewResourceFromConf(cur.Resources.Max)
if err != nil {
return err
}
- if curMaxRes.HasNegativeValue() {
- return fmt.Errorf("invalid max resource %v for queue %s, cannot be negative", curMaxRes, cur.Name)
+ if actualMax.HasNegativeValue() {
+ return fmt.Errorf("invalid max resource %v for queue %s, cannot be negative", actualMax, cur.Name)
}
+ maxToPass := resources.ComponentWiseMinPermissive(actualMax, parentMax)
+ guaranteedToPass := resources.ComponentWiseMinPermissive(maxToPass, actualGuaranteed)
+
if len(cur.Queues) > 0 {
// Check children
for i := range cur.Queues {
- if err := checkResourceConfigurationsForQueue(&cur.Queues[i], cur); err != nil {
+ if err = checkResourceConfigurationsForQueue(cur.Queues[i], maxToPass, guaranteedToPass); err != nil {
return err
}
}
- sum := resources.NewResource()
- for _, child := range cur.Queues {
- childGuaranteed, err := resources.NewResourceFromConf(child.Resources.Guaranteed)
- if err != nil {
- return err
- }
- sum.AddTo(childGuaranteed)
+ err = checkGuaranteedResource(cur.Queues, actualGuaranteed, parentGuaranteed, cur.Name)
+ if err != nil {
+ return err
}
+ }
- if cur.Resources.Guaranteed != nil {
- if !resources.FitIn(curGuaranteedRes, sum) {
- return fmt.Errorf("queue %s has guaranteed-resources (%v) smaller than sum of children guaranteed resources (%v)", cur.Name, curGuaranteedRes, sum)
- }
- } else {
- cur.Resources.Guaranteed = sum.ToConf()
+ // If max resource exist, check guaranteed fits in max, cur.max fit in parent.max
+ err = checkMaxResource(parentMax, actualMax, actualGuaranteed, cur.Name)
+ if err != nil {
+ return err
+ }
+ return nil
+}
+
+func checkGuaranteedResource(childQueues []QueueConfig, actualGuaranteed *resources.Resource, parentGuaranteed *resources.Resource, queueName string) error {
+ sum := resources.NewResource()
+ for _, child := range childQueues {
+ childGuaranteed, err := resources.NewResourceFromConf(child.Resources.Guaranteed)
+ if err != nil {
+ return err
+ }
+ sum.AddTo(childGuaranteed)
+ }
+ //the sum of guaranteed resources of all child queues of a queue may not be larger
+ //than the guaranteed resources of that queue
Review comment:
I thought that this would fail the lint test but I was wrong.
Comments however should start with a space unless there is a special reason not to (i.e. the `//nolint` comment)
----------------------------------------------------------------
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] edited a comment on pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-713539637
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=h1) Report
> Merging [#219](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=desc) (154c30b) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/01f5288eb5931a16834e0402af9efb308f946ad4?el=desc) (01f5288) will **increase** coverage by `0.02%`.
> The diff coverage is `91.48%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
+ Coverage 63.12% 63.15% +0.02%
==========================================
Files 59 59
Lines 4879 4882 +3
==========================================
+ Hits 3080 3083 +3
Misses 1657 1657
Partials 142 142
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `87.50% <85.71%> (+0.49%)` | :arrow_up: |
| [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `97.60% <100.00%> (+0.08%)` | :arrow_up: |
| [pkg/events/event\_publisher.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2V2ZW50cy9ldmVudF9wdWJsaXNoZXIuZ28=) | `88.88% <0.00%> (-11.12%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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/219?src=pr&el=footer). Last update [01f5288...f003a95](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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] wilfred-s closed pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
wilfred-s closed pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219
----------------------------------------------------------------
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] edited a comment on pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-713539637
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=h1) Report
> Merging [#219](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=desc) (c40bd9d) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5471d84cd619dc9b6c40e970b0e9d53aa3f8e07f?el=desc) (5471d84) will **increase** coverage by `0.07%`.
> The diff coverage is `86.79%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
+ Coverage 64.02% 64.10% +0.07%
==========================================
Files 60 60
Lines 4984 4992 +8
==========================================
+ Hits 3191 3200 +9
- Misses 1636 1638 +2
+ Partials 157 154 -3
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `95.32% <78.26%> (-0.71%)` | :arrow_down: |
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `88.82% <93.33%> (+1.81%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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/219?src=pr&el=footer). Last update [5471d84...c415088](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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 #219: Yunikorn 352 - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-713539611
Hey @kingamarton,
Your changes look good to me!
<a href="https://travis-ci.org/apache/incubator-yunikorn-core/builds/737697747">View build log</a>
###### TravisBuddy Request Identifier: 84538710-139a-11eb-a355-c174999d48b2
----------------------------------------------------------------
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] edited a comment on pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-713539637
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=h1) Report
> Merging [#219](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=desc) (c415088) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/361d455c7f28b64fe6ae89b7a0dca310ace7783f?el=desc) (361d455) will **increase** coverage by `1.17%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
+ Coverage 63.06% 64.24% +1.17%
==========================================
Files 60 60
Lines 5199 4992 -207
==========================================
- Hits 3279 3207 -72
+ Misses 1759 1632 -127
+ Partials 161 153 -8
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `90.00% <100.00%> (+2.99%)` | :arrow_up: |
| [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `96.78% <100.00%> (+0.75%)` | :arrow_up: |
| [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `77.77% <0.00%> (-22.23%)` | :arrow_down: |
| [pkg/common/utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi91dGlscy5nbw==) | `71.87% <0.00%> (-5.63%)` | :arrow_down: |
| [pkg/scheduler/objects/allocation\_ask.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb25fYXNrLmdv) | `90.90% <0.00%> (-2.97%)` | :arrow_down: |
| [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `6.43% <0.00%> (+0.16%)` | :arrow_up: |
| [pkg/scheduler/objects/queue.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3F1ZXVlLmdv) | `69.97% <0.00%> (+0.53%)` | :arrow_up: |
| [pkg/scheduler/objects/node.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGUuZ28=) | `83.33% <0.00%> (+1.51%)` | :arrow_up: |
| [pkg/scheduler/partition.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `66.36% <0.00%> (+3.57%)` | :arrow_up: |
| [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `55.30% <0.00%> (+6.24%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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/219?src=pr&el=footer). Last update [361d455...37ab77e](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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] kingamarton commented on a change in pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#discussion_r542330862
##########
File path: pkg/common/configs/configvalidator.go
##########
@@ -75,65 +75,100 @@ func checkACL(acl string) error {
// Check the queue resource configuration settings.
// - child or children cannot have higher maximum or guaranteed limits than parents
// - children (added together) cannot have a higher guaranteed setting than a parent
-func checkResourceConfigurationsForQueue(cur *QueueConfig, parent *QueueConfig) error {
+func checkResourceConfigurationsForQueue(cur QueueConfig, parentMax *resources.Resource, parentGuaranteed *resources.Resource) error {
Review comment:
Removed repeated parameter type
----------------------------------------------------------------
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] edited a comment on pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-713539637
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=h1) Report
> Merging [#219](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=desc) (37ab77e) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/361d455c7f28b64fe6ae89b7a0dca310ace7783f?el=desc) (361d455) will **increase** coverage by `0.21%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
+ Coverage 63.06% 63.28% +0.21%
==========================================
Files 60 60
Lines 5199 5207 +8
==========================================
+ Hits 3279 3295 +16
+ Misses 1759 1755 -4
+ Partials 161 157 -4
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `90.00% <100.00%> (+2.99%)` | :arrow_up: |
| [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `96.78% <100.00%> (+0.75%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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/219?src=pr&el=footer). Last update [361d455...37ab77e](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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] kingamarton commented on a change in pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#discussion_r542329704
##########
File path: pkg/common/configs/configvalidator.go
##########
@@ -75,65 +75,100 @@ func checkACL(acl string) error {
// Check the queue resource configuration settings.
// - child or children cannot have higher maximum or guaranteed limits than parents
// - children (added together) cannot have a higher guaranteed setting than a parent
-func checkResourceConfigurationsForQueue(cur *QueueConfig, parent *QueueConfig) error {
+func checkResourceConfigurationsForQueue(cur QueueConfig, parentMax *resources.Resource, parentGuaranteed *resources.Resource) error {
// If cur has children, make sure sum of children's guaranteed <= cur.guaranteed
- curGuaranteedRes, err := resources.NewResourceFromConf(cur.Resources.Guaranteed)
+ actualGuaranteed, err := resources.NewResourceFromConf(cur.Resources.Guaranteed)
if err != nil {
return err
}
- if curGuaranteedRes.HasNegativeValue() {
- return fmt.Errorf("invalid guaranteed resource %v for queue %s, cannot be negative", curGuaranteedRes, cur.Name)
+ if actualGuaranteed.HasNegativeValue() {
+ return fmt.Errorf("invalid guaranteed resource %v for queue %s, cannot be negative", actualGuaranteed, cur.Name)
}
- curMaxRes, err := resources.NewResourceFromConf(cur.Resources.Max)
+ actualMax, err := resources.NewResourceFromConf(cur.Resources.Max)
if err != nil {
return err
}
- if curMaxRes.HasNegativeValue() {
- return fmt.Errorf("invalid max resource %v for queue %s, cannot be negative", curMaxRes, cur.Name)
+ if actualMax.HasNegativeValue() {
+ return fmt.Errorf("invalid max resource %v for queue %s, cannot be negative", actualMax, cur.Name)
}
+ maxToPass := resources.ComponentWiseMinPermissive(actualMax, parentMax)
+ guaranteedToPass := resources.ComponentWiseMinPermissive(maxToPass, actualGuaranteed)
+
if len(cur.Queues) > 0 {
// Check children
for i := range cur.Queues {
- if err := checkResourceConfigurationsForQueue(&cur.Queues[i], cur); err != nil {
+ if err = checkResourceConfigurationsForQueue(cur.Queues[i], maxToPass, guaranteedToPass); err != nil {
return err
}
}
- sum := resources.NewResource()
- for _, child := range cur.Queues {
- childGuaranteed, err := resources.NewResourceFromConf(child.Resources.Guaranteed)
- if err != nil {
- return err
- }
- sum.AddTo(childGuaranteed)
+ err = checkGuaranteedResource(cur.Queues, actualGuaranteed, parentGuaranteed, cur.Name)
+ if err != nil {
+ return err
}
+ }
- if cur.Resources.Guaranteed != nil {
- if !resources.FitIn(curGuaranteedRes, sum) {
- return fmt.Errorf("queue %s has guaranteed-resources (%v) smaller than sum of children guaranteed resources (%v)", cur.Name, curGuaranteedRes, sum)
- }
- } else {
- cur.Resources.Guaranteed = sum.ToConf()
Review comment:
reverted that func
----------------------------------------------------------------
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] edited a comment on pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-713539637
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=h1) Report
> Merging [#219](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=desc) (2167ed9) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/01f5288eb5931a16834e0402af9efb308f946ad4?el=desc) (01f5288) will **decrease** coverage by `0.18%`.
> The diff coverage is `65.95%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
- Coverage 63.12% 62.94% -0.19%
==========================================
Files 59 59
Lines 4879 4882 +3
==========================================
- Hits 3080 3073 -7
- Misses 1657 1661 +4
- Partials 142 148 +6
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `94.01% <36.84%> (-3.51%)` | :arrow_down: |
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `87.50% <85.71%> (+0.49%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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/219?src=pr&el=footer). Last update [01f5288...2167ed9](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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] wilfred-s commented on pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-745116652
The patch looks good beside the codecov remarks.
I can see that the error or no input cases are not hit based on the remarks it puts in. Specially in the resources `ComponentWiseMinPermissive()` we can get a couple easily fixed by nil testing. However this is a central part of the code and we should really strive for a higher test coverage.
----------------------------------------------------------------
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] edited a comment on pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-713539637
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=h1) Report
> Merging [#219](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=desc) (572c116) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/01f5288eb5931a16834e0402af9efb308f946ad4?el=desc) (01f5288) will **decrease** coverage by `0.26%`.
> The diff coverage is `57.14%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
- Coverage 63.12% 62.86% -0.27%
==========================================
Files 59 59
Lines 4879 4885 +6
==========================================
- Hits 3080 3071 -9
- Misses 1657 1665 +8
- Partials 142 149 +7
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `92.58% <19.04%> (-4.94%)` | :arrow_down: |
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `87.50% <85.71%> (+0.49%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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/219?src=pr&el=footer). Last update [01f5288...572c116](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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] codecov[bot] edited a comment on pull request #219: [YUNIKORN-352] - fixed max resource check and behavioural change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #219:
URL: https://github.com/apache/incubator-yunikorn-core/pull/219#issuecomment-713539637
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=h1) Report
> Merging [#219](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=desc) (154c30b) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5471d84cd619dc9b6c40e970b0e9d53aa3f8e07f?el=desc) (5471d84) will **decrease** coverage by `0.87%`.
> The diff coverage is `91.48%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
- Coverage 64.02% 63.15% -0.88%
==========================================
Files 60 59 -1
Lines 4984 4882 -102
==========================================
- Hits 3191 3083 -108
- Misses 1636 1657 +21
+ Partials 157 142 -15
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `87.50% <85.71%> (+0.49%)` | :arrow_up: |
| [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `97.60% <100.00%> (+1.58%)` | :arrow_up: |
| [pkg/scheduler/partition\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb25fbWFuYWdlci5nbw==) | `0.00% <0.00%> (-16.00%)` | :arrow_down: |
| [pkg/events/event\_publisher.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL2V2ZW50cy9ldmVudF9wdWJsaXNoZXIuZ28=) | `88.88% <0.00%> (-11.12%)` | :arrow_down: |
| [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `0.00% <0.00%> (-6.44%)` | :arrow_down: |
| [pkg/scheduler/partition.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `64.90% <0.00%> (-1.46%)` | :arrow_down: |
| [pkg/metrics/init.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL21ldHJpY3MvaW5pdC5nbw==) | `62.96% <0.00%> (ø)` | |
| [pkg/scheduler/health\_checker.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9oZWFsdGhfY2hlY2tlci5nbw==) | | |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `55.08% <0.00%> (+0.94%)` | :arrow_up: |
| ... and [1 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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/219?src=pr&el=footer). Last update [5471d84...ad72199](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/219?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