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/22 21:16:57 UTC
[GitHub] [incubator-yunikorn-k8shim] yangwwei opened a new pull request #200: [YUNIKORN-421] Define app gang scheduling info in API package.
yangwwei opened a new pull request #200:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/200
----------------------------------------------------------------
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-k8shim] kingamarton commented on a change in pull request #200: [YUNIKORN-421] Define app gang scheduling info in API package.
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #200:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/200#discussion_r511602817
##########
File path: test/e2e/testdata/application.yaml
##########
@@ -24,9 +24,11 @@ spec:
name: TryOnce
queue: root.default
taskGroups:
- - groupName: "test-task-0001"
- minMember: 1
- minResource:
- cpu: "300m"
- memory: "128Mi"
+ - schedulingPolicy: "TryReserve"
+ - groups:
Review comment:
I liked better the previous structure of having just taskGroups field instead of having taskGroups, than groups. For me it seems redundant to have both fields. Can you please explain why it is better this structure than the previous one?
##########
File path: test/e2e/app/app_test.go
##########
@@ -74,14 +74,14 @@ var _ = ginkgo.Describe("App", func() {
gomega.Ω(appCRD.Spec.Queue).To(gomega.Equal("root.default"))
gomega.Ω(appCRD.ObjectMeta.Name).To(gomega.Equal("example"))
gomega.Ω(appCRD.ObjectMeta.Namespace).To(gomega.Equal(dev))
- policy := appCRD.Spec.Policy.Policy
- gomega.Ω(string(policy)).To(gomega.Equal("TryOnce"))
- gomega.Ω(appCRD.Spec.TaskGroup[0].GroupName).To(gomega.Equal("test-task-0001"))
- gomega.Ω(appCRD.Spec.TaskGroup[0].MinMember).To(gomega.Equal(int32(1)))
+ policy := appCRD.Spec.TaskGroups.SchedulingPolicy
+ gomega.Ω(policy.Type).To(gomega.Equal("TryReserve"))
Review comment:
Please use the TryReserve constant defined for the policy 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-k8shim] yangwwei commented on a change in pull request #200: [YUNIKORN-421] Define app gang scheduling info in API package.
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #200:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/200#discussion_r512434308
##########
File path: test/e2e/app/app_test.go
##########
@@ -74,14 +74,14 @@ var _ = ginkgo.Describe("App", func() {
gomega.Ω(appCRD.Spec.Queue).To(gomega.Equal("root.default"))
gomega.Ω(appCRD.ObjectMeta.Name).To(gomega.Equal("example"))
gomega.Ω(appCRD.ObjectMeta.Namespace).To(gomega.Equal(dev))
- policy := appCRD.Spec.Policy.Policy
- gomega.Ω(string(policy)).To(gomega.Equal("TryOnce"))
- gomega.Ω(appCRD.Spec.TaskGroup[0].GroupName).To(gomega.Equal("test-task-0001"))
- gomega.Ω(appCRD.Spec.TaskGroup[0].MinMember).To(gomega.Equal(int32(1)))
+ policy := appCRD.Spec.TaskGroups.SchedulingPolicy
+ gomega.Ω(policy.Type).To(gomega.Equal("TryReserve"))
Review comment:
Make sense, 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-k8shim] codecov[bot] commented on pull request #200: [YUNIKORN-421] Define app gang scheduling info in API package.
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #200:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/200#issuecomment-717528920
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/200?src=pr&el=h1) Report
> Merging [#200](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/200?src=pr&el=desc) into [YUNIKORN-2](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c2106761cea7109beec15efb321baa1ee19a5658?el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/200?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## YUNIKORN-2 #200 +/- ##
===========================================
Coverage 58.29% 58.29%
===========================================
Files 33 33
Lines 2928 2928
===========================================
Hits 1707 1707
Misses 1149 1149
Partials 72 72
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/200?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-k8shim/pull/200?src=pr&el=footer). Last update [c210676...ea9c087](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/200?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-k8shim] yangwwei commented on a change in pull request #200: [YUNIKORN-421] Define app gang scheduling info in API package.
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #200:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/200#discussion_r512434381
##########
File path: test/e2e/testdata/application.yaml
##########
@@ -24,9 +24,11 @@ spec:
name: TryOnce
queue: root.default
taskGroups:
- - groupName: "test-task-0001"
- minMember: 1
- minResource:
- cpu: "300m"
- memory: "128Mi"
+ - schedulingPolicy: "TryReserve"
+ - groups:
Review comment:
I've changed it back. pls take a look.
----------------------------------------------------------------
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-k8shim] yangwwei merged pull request #200: [YUNIKORN-421] Define app gang scheduling info in API package.
Posted by GitBox <gi...@apache.org>.
yangwwei merged pull request #200:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/200
----------------------------------------------------------------
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