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 2022/12/12 23:49:08 UTC
[GitHub] [yunikorn-core] bgrams opened a new pull request, #474: [YUNIKORN-1468] Remove completedApplications from Queue
bgrams opened a new pull request, #474:
URL: https://github.com/apache/yunikorn-core/pull/474
### What is this PR for?
As described in the Jira, this is a more comprehensive fix for the issues identified via YUNIKORN-1440 and discussed in #471. Proposed changes ensure that app history and cleanup mechanisms are solely managed at the Partition level. Existing behavior for the `/ws/v1/partition/{partition}/queue/{queue}` REST endpoint remains unchanged.
### What type of PR is it?
* [x] - Bug Fix
* [ ] - Improvement
* [ ] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-1468
### How should this be tested?
Tests modified but largely unchanged.
### Screenshots (if appropriate)
### Questions:
* [ ] - The licenses files need update.
* [ ] - There is breaking changes for older versions.
* [ ] - It needs documentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] bgrams commented on a diff in pull request #474: [YUNIKORN-1468] Remove completedApplications from Queue
Posted by GitBox <gi...@apache.org>.
bgrams commented on code in PR #474:
URL: https://github.com/apache/yunikorn-core/pull/474#discussion_r1047409520
##########
pkg/webservice/handlers.go:
##########
@@ -534,15 +534,18 @@ func getQueueApplications(w http.ResponseWriter, r *http.Request) {
buildJSONErrorResponse(w, QueueDoesNotExists, http.StatusBadRequest)
return
}
- apps := queue.GetCopyOfApps()
- completedApps := queue.GetCopyOfCompletedApps()
- appsDao := make([]*dao.ApplicationDAOInfo, 0, len(apps)+len(completedApps))
- for _, app := range apps {
+
+ appsDao := make([]*dao.ApplicationDAOInfo, 0)
+ for _, app := range queue.GetCopyOfApps() {
appsDao = append(appsDao, getApplicationJSON(app))
}
- for _, app := range completedApps {
- appsDao = append(appsDao, getApplicationJSON(app))
+
+ for _, app := range partitionContext.GetCompletedApplications() {
+ if app.GetQueuePath() == queueName {
+ appsDao = append(appsDao, getApplicationJSON(app))
+ }
}
+
Review Comment:
SGTM. Addressed in latest commits.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] codecov[bot] commented on pull request #474: [YUNIKORN-1468] Remove completedApplications from Queue
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #474:
URL: https://github.com/apache/yunikorn-core/pull/474#issuecomment-1349003575
# [Codecov](https://codecov.io/gh/apache/yunikorn-core/pull/474?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#474](https://codecov.io/gh/apache/yunikorn-core/pull/474?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d0aaddd) into [master](https://codecov.io/gh/apache/yunikorn-core/commit/020bbd8544fd29743b2284e473695221de552826?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (020bbd8) will **decrease** coverage by `0.41%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #474 +/- ##
==========================================
- Coverage 72.88% 72.47% -0.42%
==========================================
Files 67 67
Lines 10057 10095 +38
==========================================
- Hits 7330 7316 -14
- Misses 2482 2533 +51
- Partials 245 246 +1
```
| [Impacted Files](https://codecov.io/gh/apache/yunikorn-core/pull/474?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [pkg/scheduler/objects/queue.go](https://codecov.io/gh/apache/yunikorn-core/pull/474/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3F1ZXVlLmdv) | `70.22% <ø> (-0.10%)` | :arrow_down: |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/yunikorn-core/pull/474/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `77.30% <100.00%> (-0.18%)` | :arrow_down: |
| [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/yunikorn-core/pull/474/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `56.12% <0.00%> (-2.07%)` | :arrow_down: |
| [pkg/scheduler/partition.go](https://codecov.io/gh/apache/yunikorn-core/pull/474/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `77.15% <0.00%> (-0.59%)` | :arrow_down: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #474: [YUNIKORN-1468] Remove completedApplications from Queue
Posted by GitBox <gi...@apache.org>.
craigcondit commented on code in PR #474:
URL: https://github.com/apache/yunikorn-core/pull/474#discussion_r1047352537
##########
pkg/webservice/handlers.go:
##########
@@ -534,15 +534,18 @@ func getQueueApplications(w http.ResponseWriter, r *http.Request) {
buildJSONErrorResponse(w, QueueDoesNotExists, http.StatusBadRequest)
return
}
- apps := queue.GetCopyOfApps()
- completedApps := queue.GetCopyOfCompletedApps()
- appsDao := make([]*dao.ApplicationDAOInfo, 0, len(apps)+len(completedApps))
- for _, app := range apps {
+
+ appsDao := make([]*dao.ApplicationDAOInfo, 0)
+ for _, app := range queue.GetCopyOfApps() {
appsDao = append(appsDao, getApplicationJSON(app))
}
- for _, app := range completedApps {
- appsDao = append(appsDao, getApplicationJSON(app))
+
+ for _, app := range partitionContext.GetCompletedApplications() {
+ if app.GetQueuePath() == queueName {
+ appsDao = append(appsDao, getApplicationJSON(app))
+ }
}
+
Review Comment:
Agreed. We can deprecate the field in the 1.2 docs, and remove in 2.0. It will just be empty until then.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] wilfred-s commented on a diff in pull request #474: [YUNIKORN-1468] Remove completedApplications from Queue
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on code in PR #474:
URL: https://github.com/apache/yunikorn-core/pull/474#discussion_r1046730272
##########
pkg/webservice/handlers.go:
##########
@@ -534,15 +534,18 @@ func getQueueApplications(w http.ResponseWriter, r *http.Request) {
buildJSONErrorResponse(w, QueueDoesNotExists, http.StatusBadRequest)
return
}
- apps := queue.GetCopyOfApps()
- completedApps := queue.GetCopyOfCompletedApps()
- appsDao := make([]*dao.ApplicationDAOInfo, 0, len(apps)+len(completedApps))
- for _, app := range apps {
+
+ appsDao := make([]*dao.ApplicationDAOInfo, 0)
+ for _, app := range queue.GetCopyOfApps() {
appsDao = append(appsDao, getApplicationJSON(app))
}
- for _, app := range completedApps {
- appsDao = append(appsDao, getApplicationJSON(app))
+
+ for _, app := range partitionContext.GetCompletedApplications() {
+ if app.GetQueuePath() == queueName {
+ appsDao = append(appsDao, getApplicationJSON(app))
+ }
}
+
Review Comment:
I would suggest that we remove that and add a release note for the same to YuniKorn 1.2 explaining the reasoning behind the change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit closed pull request #474: [YUNIKORN-1468] Remove completedApplications from Queue
Posted by GitBox <gi...@apache.org>.
craigcondit closed pull request #474: [YUNIKORN-1468] Remove completedApplications from Queue
URL: https://github.com/apache/yunikorn-core/pull/474
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org