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/06/03 15:44:02 UTC
[GitHub] [incubator-yunikorn-core] wilfred-s opened a new pull request #165: [YUNIKORN-199] Set unmanaged queue max resource
wilfred-s opened a new pull request #165:
URL: https://github.com/apache/incubator-yunikorn-core/pull/165
Set the unmanaged max queue resource based on the tag that is passed in
with a new application. The tag represents the quota from a namespace
from k8s. The max resource is only set on an unmanaged leaf queue as
the functionality should be used with the tag based placement rule.
This works around the hard namespace quota in k8s without the need for
major changes on the side of k8s namespace management.
The tag that is used is "namespace.resourcequota".
----------------------------------------------------------------
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 #165: [YUNIKORN-199] Set unmanaged queue max resource
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on pull request #165:
URL: https://github.com/apache/incubator-yunikorn-core/pull/165#issuecomment-638299779
The lint issues have been fixed but there is a new race condition detected which is logged as YUNIKORN-202. Again around event processing and the logging that happens (similar to YUNIKORN-155)
----------------------------------------------------------------
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 #165: [YUNIKORN-203] Set unmanaged queue max resource
Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #165:
URL: https://github.com/apache/incubator-yunikorn-core/pull/165#issuecomment-638743064
Hey @wilfred-s,
Your changes look good to me!
<a href="https://travis-ci.org/apache/incubator-yunikorn-core/builds/694569821">View build log</a>
###### TravisBuddy Request Identifier: 33db2f00-a648-11ea-9311-0306711df575
----------------------------------------------------------------
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 #165: [YUNIKORN-203] Set unmanaged queue max resource
Posted by GitBox <gi...@apache.org>.
wilfred-s closed pull request #165:
URL: https://github.com/apache/incubator-yunikorn-core/pull/165
----------------------------------------------------------------
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 #165: [YUNIKORN-199] Set unmanaged queue max resource
Posted by GitBox <gi...@apache.org>.
TravisBuddy commented on pull request #165:
URL: https://github.com/apache/incubator-yunikorn-core/pull/165#issuecomment-638283834
## 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/694312661">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_test.go:1362:3: var `fromJson` should be `fromJSON` (golint)
fromJson, err := NewResourceFromString(test.jsonRes)
^
pkg/common/resources/resources_test.go:1316: File is not `goimports`-ed with -local github.com/apache/incubator-yunikorn (goimports)
jsonRes string
fail bool
Makefile:45: recipe for target 'lint' failed
make: *** [lint] Error 1
```
</details>
###### TravisBuddy Request Identifier: 847620b0-a5b1-11ea-84d7-9d6761bd5292
----------------------------------------------------------------
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 #165: [YUNIKORN-203] Set unmanaged queue max resource
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #165:
URL: https://github.com/apache/incubator-yunikorn-core/pull/165#discussion_r435041305
##########
File path: pkg/common/resources/resources.go
##########
@@ -66,6 +67,16 @@ func NewResourceFromMap(m map[string]Quantity) *Resource {
return &Resource{Resources: m}
}
+// Create a new resource from a string.
+// The string must be a json marshalled list of map[string]string.
+func NewResourceFromString(str string) (*Resource, error) {
+ var resMap map[string]string
Review comment:
I used the wrong format to unmarshal, I had the inner set not the outer set. so yes we should go through the `si.Resource`
Fixed that in the new commit
----------------------------------------------------------------
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-commenter commented on pull request #165: [YUNIKORN-203] Set unmanaged queue max resource
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #165:
URL: https://github.com/apache/incubator-yunikorn-core/pull/165#issuecomment-638743110
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/165?src=pr&el=h1) Report
> Merging [#165](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/165?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/772e3cbdbdbda397468f19e09f137433caf5f144&el=desc) will **increase** coverage by `0.25%`.
> The diff coverage is `82.75%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/165?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #165 +/- ##
==========================================
+ Coverage 58.69% 58.95% +0.25%
==========================================
Files 62 62
Lines 6179 6208 +29
==========================================
+ Hits 3627 3660 +33
+ Misses 2411 2406 -5
- Partials 141 142 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/165?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/scheduler/scheduling\_queue.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/165/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9zY2hlZHVsaW5nX3F1ZXVlLmdv) | `87.13% <64.28%> (-1.08%)` | :arrow_down: |
| [pkg/cache/queue\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/165/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3F1ZXVlX2luZm8uZ28=) | `70.45% <100.00%> (+0.92%)` | :arrow_up: |
| [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/165/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `96.28% <100.00%> (+0.04%)` | :arrow_up: |
| [pkg/scheduler/scheduling\_application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/165/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9zY2hlZHVsaW5nX2FwcGxpY2F0aW9uLmdv) | `73.24% <100.00%> (+0.13%)` | :arrow_up: |
| [pkg/cache/application\_info.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/165/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2luZm8uZ28=) | `80.37% <0.00%> (+8.41%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/165?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/165?src=pr&el=footer). Last update [772e3cb...52b0bd7](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/165?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] yangwwei commented on a change in pull request #165: [YUNIKORN-203] Set unmanaged queue max resource
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #165:
URL: https://github.com/apache/incubator-yunikorn-core/pull/165#discussion_r434986292
##########
File path: pkg/common/resources/resources.go
##########
@@ -66,6 +67,16 @@ func NewResourceFromMap(m map[string]Quantity) *Resource {
return &Resource{Resources: m}
}
+// Create a new resource from a string.
+// The string must be a json marshalled list of map[string]string.
+func NewResourceFromString(str string) (*Resource, error) {
+ var resMap map[string]string
Review comment:
can we directly unmarshal the string to `*si.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