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 2021/12/15 16:28:02 UTC
[GitHub] [incubator-yunikorn-core] manirajv06 opened a new pull request #350: YUNIKORN-462: Streamline core to shim update on allocation change
manirajv06 opened a new pull request #350:
URL: https://github.com/apache/incubator-yunikorn-core/pull/350
### What is this PR for?
New Allocation and Release allocations events has been changed from async to synchronous communication so that shim updates its own cache immediately followed up by pod bind/delete steps. With this change, ReSyncSchedulerCache plugin doesn't have any more role to play. Hence, cleaned up the related plugin code as well.
### What type of PR is it?
* [ ] - Improvement
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-462
### How should this be tested?
### 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] [incubator-yunikorn-core] wilfred-s closed pull request #350: YUNIKORN-462: Streamline core to shim update on allocation change
Posted by GitBox <gi...@apache.org>.
wilfred-s closed pull request #350:
URL: https://github.com/apache/incubator-yunikorn-core/pull/350
--
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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #350: YUNIKORN-462: Streamline core to shim update on allocation change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #350:
URL: https://github.com/apache/incubator-yunikorn-core/pull/350#issuecomment-1046227183
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?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 [#350](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b1f6b90) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/74f9743dab2dae12808eb5b9a075aad9f19107fd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (74f9743) will **increase** coverage by `0.11%`.
> The diff coverage is `0.00%`.
> :exclamation: Current head b1f6b90 differs from pull request most recent head b185971. Consider uploading reports for the commit b185971 to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #350 +/- ##
==========================================
+ Coverage 68.21% 68.33% +0.11%
==========================================
Files 65 65
Lines 9099 9079 -20
==========================================
- Hits 6207 6204 -3
+ Misses 2653 2636 -17
Partials 239 239
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?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/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350/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-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `29.55% <0.00%> (+0.80%)` | :arrow_up: |
| [pkg/scheduler/tests/mock\_rm\_callback.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350/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-cGtnL3NjaGVkdWxlci90ZXN0cy9tb2NrX3JtX2NhbGxiYWNrLmdv) | `26.66% <ø> (-12.23%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [74f9743...b185971](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #350: YUNIKORN-462: Streamline core to shim update on allocation change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #350:
URL: https://github.com/apache/incubator-yunikorn-core/pull/350#issuecomment-1046227183
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?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 [#350](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0f9a00e) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/74f9743dab2dae12808eb5b9a075aad9f19107fd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (74f9743) will **increase** coverage by `0.16%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #350 +/- ##
==========================================
+ Coverage 68.21% 68.38% +0.16%
==========================================
Files 65 65
Lines 9099 9072 -27
==========================================
- Hits 6207 6204 -3
+ Misses 2653 2629 -24
Partials 239 239
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?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/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350/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-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `29.90% <0.00%> (+1.14%)` | :arrow_up: |
| [pkg/scheduler/tests/mock\_rm\_callback.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350/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-cGtnL3NjaGVkdWxlci90ZXN0cy9tb2NrX3JtX2NhbGxiYWNrLmdv) | `26.66% <ø> (-12.23%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [74f9743...0f9a00e](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #350: YUNIKORN-462: Streamline core to shim update on allocation change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #350:
URL: https://github.com/apache/incubator-yunikorn-core/pull/350#issuecomment-1046227183
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?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 [#350](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b1f6b90) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/74f9743dab2dae12808eb5b9a075aad9f19107fd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (74f9743) will **increase** coverage by `0.11%`.
> The diff coverage is `0.00%`.
> :exclamation: Current head b1f6b90 differs from pull request most recent head bed4861. Consider uploading reports for the commit bed4861 to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #350 +/- ##
==========================================
+ Coverage 68.21% 68.33% +0.11%
==========================================
Files 65 65
Lines 9099 9079 -20
==========================================
- Hits 6207 6204 -3
+ Misses 2653 2636 -17
Partials 239 239
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?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/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350/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-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `29.55% <0.00%> (+0.80%)` | :arrow_up: |
| [pkg/scheduler/tests/mock\_rm\_callback.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350/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-cGtnL3NjaGVkdWxlci90ZXN0cy9tb2NrX3JtX2NhbGxiYWNrLmdv) | `26.66% <ø> (-12.23%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [74f9743...bed4861](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-yunikorn-core] wilfred-s commented on a change in pull request #350: YUNIKORN-462: Streamline core to shim update on allocation change
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #350:
URL: https://github.com/apache/incubator-yunikorn-core/pull/350#discussion_r810714287
##########
File path: go.mod
##########
@@ -23,7 +23,7 @@ go 1.16
require (
github.com/HdrHistogram/hdrhistogram-go v1.0.1 // indirect
- github.com/apache/incubator-yunikorn-scheduler-interface v0.0.0-20220214023648-0de002ac41d3
+ github.com/apache/incubator-yunikorn-scheduler-interface v0.11.1-0.20220218043513-e19a1b0c381f
Review comment:
This is pointing to a wrong pseudo version. The 0.11.1 should be 0.0.0. The psuedo version can be easily generated with this one line script:
```
TZ=UTC git --no-pager show --quiet --abbrev=12 --date='format-local:%Y%m%d%H%M%S' --format='v0.0.0-%cd-%h'
```
##########
File path: pkg/rmproxy/rmproxy.go
##########
@@ -97,14 +97,15 @@ func (rmp *RMProxy) handleUpdateResponseError(rmID string, err error) {
func (rmp *RMProxy) processAllocationUpdateEvent(event *rmevent.RMNewAllocationsEvent) {
rmp.RLock()
defer rmp.RUnlock()
- if len(event.Allocations) == 0 {
- return
- }
Review comment:
We must send a response back and should not rely on the caller to do the right thing.
If we introduce a bug or remove the optimisation in the caller side things could hang indefinitely as there is no response coming back on the event channel if the length of the allocations in the event is 0. We can even postpone the locking to inside the `if len()` statement and make it an unlocked process if nothing is 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.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
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 #350: YUNIKORN-462: Streamline core to shim update on allocation change
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #350:
URL: https://github.com/apache/incubator-yunikorn-core/pull/350#discussion_r809683356
##########
File path: pkg/scheduler/context.go
##########
@@ -840,6 +797,7 @@ func (cc *ClusterContext) notifyRMAllocationReleased(rmID string, released []*ob
releaseEvent := &rmevent.RMReleaseAllocationEvent{
ReleasedAllocations: make([]*si.AllocationRelease, 0),
RmID: rmID,
+ Channel: nil,
Review comment:
This should no longer be the case based on the SI discussions.
##########
File path: pkg/rmproxy/rmproxy.go
##########
@@ -97,14 +97,15 @@ func (rmp *RMProxy) handleUpdateResponseError(rmID string, err error) {
func (rmp *RMProxy) processAllocationUpdateEvent(event *rmevent.RMNewAllocationsEvent) {
rmp.RLock()
defer rmp.RUnlock()
- if len(event.Allocations) == 0 {
- return
- }
Review comment:
I think we can leave this optimisation in the code: if no allocations are given we should return the success immediately otherwise process:
```
if len(event.Allocations) != 0 {
// do what we need to do
}
// notify the channel
```
##########
File path: pkg/rmproxy/rmproxy.go
##########
@@ -139,14 +140,18 @@ func (rmp *RMProxy) processApplicationUpdateEvent(event *rmevent.RMApplicationUp
func (rmp *RMProxy) processRMReleaseAllocationEvent(event *rmevent.RMReleaseAllocationEvent) {
rmp.RLock()
defer rmp.RUnlock()
- if len(event.ReleasedAllocations) == 0 {
- return
- }
response := &si.AllocationResponse{
Released: event.ReleasedAllocations,
}
rmp.triggerUpdateAllocation(event.RmID, response)
metrics.GetSchedulerMetrics().AddReleasedContainers(len(event.ReleasedAllocations))
+
+ // Done, notify channel
+ if event.Channel != nil {
Review comment:
Is it still the case that the channel can be nil after the updates we have discussed?
##########
File path: pkg/rmproxy/rmproxy.go
##########
@@ -139,14 +140,18 @@ func (rmp *RMProxy) processApplicationUpdateEvent(event *rmevent.RMApplicationUp
func (rmp *RMProxy) processRMReleaseAllocationEvent(event *rmevent.RMReleaseAllocationEvent) {
rmp.RLock()
defer rmp.RUnlock()
- if len(event.ReleasedAllocations) == 0 {
- return
- }
Review comment:
Same as above use the optimisation
--
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] [incubator-yunikorn-core] manirajv06 commented on pull request #350: YUNIKORN-462: Streamline core to shim update on allocation change
Posted by GitBox <gi...@apache.org>.
manirajv06 commented on pull request #350:
URL: https://github.com/apache/incubator-yunikorn-core/pull/350#issuecomment-998775090
Have made changes in SI as well. If you can take a look at https://github.com/apache/incubator-yunikorn-scheduler-interface/pull/58 first before core and shim's pr, then whole changes gives us clear picture and easy for us to fix these lint issues.
--
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] [incubator-yunikorn-core] codecov[bot] commented on pull request #350: YUNIKORN-462: Streamline core to shim update on allocation change
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #350:
URL: https://github.com/apache/incubator-yunikorn-core/pull/350#issuecomment-1046227183
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?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 [#350](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b1f6b90) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/74f9743dab2dae12808eb5b9a075aad9f19107fd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (74f9743) will **increase** coverage by `0.11%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #350 +/- ##
==========================================
+ Coverage 68.21% 68.33% +0.11%
==========================================
Files 65 65
Lines 9099 9079 -20
==========================================
- Hits 6207 6204 -3
+ Misses 2653 2636 -17
Partials 239 239
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?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/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350/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-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `29.55% <0.00%> (+0.80%)` | :arrow_up: |
| [pkg/scheduler/tests/mock\_rm\_callback.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350/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-cGtnL3NjaGVkdWxlci90ZXN0cy9tb2NrX3JtX2NhbGxiYWNrLmdv) | `26.66% <ø> (-12.23%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [74f9743...b1f6b90](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-yunikorn-core] wilfred-s commented on a change in pull request #350: YUNIKORN-462: Streamline core to shim update on allocation change
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #350:
URL: https://github.com/apache/incubator-yunikorn-core/pull/350#discussion_r779184329
##########
File path: pkg/rmproxy/rmevent/events.go
##########
@@ -88,6 +88,12 @@ type RMReleaseAllocationEvent struct {
ReleasedAllocations []*si.AllocationRelease
}
+type RMReleaseAllocationSyncEvent struct {
+ RmID string
+ ReleasedAllocations []*si.AllocationRelease
+ Channel chan *Result
+}
+
Review comment:
Can we hide this in the logic? Have only one event (not a sync and async version).
For a sync event you add a channel in the caller and wait for the response on the channel. For the async processing we can pass in a nil channel. Make the proxy handle it. I think it will allow us to remove code duplication.
##########
File path: pkg/rmproxy/rmproxy.go
##########
@@ -98,13 +98,21 @@ func (rmp *RMProxy) processAllocationUpdateEvent(event *rmevent.RMNewAllocations
rmp.RLock()
defer rmp.RUnlock()
if len(event.Allocations) == 0 {
+ event.Channel <- &rmevent.Result{
+ Reason: "RMNewAllocationsEvent has zero allocations" ,
+ Succeeded: false,
+ }
Review comment:
an empty list of allocations should not be a failure that changes the current behaviour we have now
--
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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #350: YUNIKORN-462: Streamline core to shim update on allocation change
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #350:
URL: https://github.com/apache/incubator-yunikorn-core/pull/350#issuecomment-1046227183
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?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 [#350](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b1f6b90) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/74f9743dab2dae12808eb5b9a075aad9f19107fd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (74f9743) will **increase** coverage by `0.11%`.
> The diff coverage is `0.00%`.
> :exclamation: Current head b1f6b90 differs from pull request most recent head 0f9a00e. Consider uploading reports for the commit 0f9a00e to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #350 +/- ##
==========================================
+ Coverage 68.21% 68.33% +0.11%
==========================================
Files 65 65
Lines 9099 9079 -20
==========================================
- Hits 6207 6204 -3
+ Misses 2653 2636 -17
Partials 239 239
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?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/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350/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-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `29.55% <0.00%> (+0.80%)` | :arrow_up: |
| [pkg/scheduler/tests/mock\_rm\_callback.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350/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-cGtnL3NjaGVkdWxlci90ZXN0cy9tb2NrX3JtX2NhbGxiYWNrLmdv) | `26.66% <ø> (-12.23%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [74f9743...0f9a00e](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/350?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-yunikorn-core] manirajv06 commented on a change in pull request #350: YUNIKORN-462: Streamline core to shim update on allocation change
Posted by GitBox <gi...@apache.org>.
manirajv06 commented on a change in pull request #350:
URL: https://github.com/apache/incubator-yunikorn-core/pull/350#discussion_r810048976
##########
File path: pkg/rmproxy/rmproxy.go
##########
@@ -97,14 +97,15 @@ func (rmp *RMProxy) handleUpdateResponseError(rmID string, err error) {
func (rmp *RMProxy) processAllocationUpdateEvent(event *rmevent.RMNewAllocationsEvent) {
rmp.RLock()
defer rmp.RUnlock()
- if len(event.Allocations) == 0 {
- return
- }
Review comment:
This has been already handled in caller side. For example notifyRMNewAllocation would be called only if it is necessary. Added extra check for the same in notifyRMNewAllocation to make it more safer. Also, sending it as success if case of no allocations gives wrong impression on the caller side. Anyways, I think caller side code is good enough.
--
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] [incubator-yunikorn-core] manirajv06 commented on a change in pull request #350: YUNIKORN-462: Streamline core to shim update on allocation change
Posted by GitBox <gi...@apache.org>.
manirajv06 commented on a change in pull request #350:
URL: https://github.com/apache/incubator-yunikorn-core/pull/350#discussion_r810050738
##########
File path: pkg/rmproxy/rmproxy.go
##########
@@ -139,14 +140,18 @@ func (rmp *RMProxy) processApplicationUpdateEvent(event *rmevent.RMApplicationUp
func (rmp *RMProxy) processRMReleaseAllocationEvent(event *rmevent.RMReleaseAllocationEvent) {
rmp.RLock()
defer rmp.RUnlock()
- if len(event.ReleasedAllocations) == 0 {
- return
- }
Review comment:
Caller side already had this check ( len(releaseEvent.ReleasedAllocations) ) but now I moved the same to beginning of the method.
--
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] [incubator-yunikorn-core] wilfred-s commented on pull request #350: YUNIKORN-462: Streamline core to shim update on allocation change
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on pull request #350:
URL: https://github.com/apache/incubator-yunikorn-core/pull/350#issuecomment-998519682
Please check why the linter cannot build the code. The change seems to reference a non existing field. This might be a typo or wrong version of the interface etc.
--
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