Re: [PR] [YUNIKORN-2204] Use ask unique id for allocation [yunikorn-core]
wilfred-s closed pull request #740: [YUNIKORN-2204] Use ask unique id for allocation URL: https://github.com/apache/yunikorn-core/pull/740 -- 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-unsubscr...@yunikorn.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [YUNIKORN-2204] Use ask unique id for allocation [yunikorn-core]
wilfred-s commented on code in PR #740:
URL: https://github.com/apache/yunikorn-core/pull/740#discussion_r1410334191
##
pkg/scheduler/objects/allocation.go:
##
@@ -152,7 +154,9 @@ func NewAllocationFromSI(alloc *si.Allocation) *Allocation {
createTime:time.Unix(creationTime, 0),
allocLog: make(map[string]*AllocationLogEntry),
}
- return NewAllocation(alloc.UUID, alloc.NodeID, ask)
+ newAlloc := NewAllocation(alloc.NodeID, ask)
+ newAlloc.uuid = alloc.UUID
+ return newAlloc
Review Comment:
Please file a follow up jira on this as we need to properly handle this
case. However we need to wait until we have the initialisation code done on the
shim side.
--
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-unsubscr...@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [YUNIKORN-2204] Use ask unique id for allocation [yunikorn-core]
manirajv06 commented on PR #740: URL: https://github.com/apache/yunikorn-core/pull/740#issuecomment-1833167456 > There should be no new calls to `GetUUID()` everything should be using the new `GetAllocationID()` If you update a line that already had a `GetUUID` call it should be replaced with a `GetAllocationID` call. So we do not touch the line again when removing the `GetUUUID` function. 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-unsubscr...@yunikorn.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [YUNIKORN-2204] Use ask unique id for allocation [yunikorn-core]
manirajv06 commented on code in PR #740:
URL: https://github.com/apache/yunikorn-core/pull/740#discussion_r1409083589
##
pkg/scheduler/objects/allocation.go:
##
@@ -152,7 +154,9 @@ func NewAllocationFromSI(alloc *si.Allocation) *Allocation {
createTime:time.Unix(creationTime, 0),
allocLog: make(map[string]*AllocationLogEntry),
}
- return NewAllocation(alloc.UUID, alloc.NodeID, ask)
+ newAlloc := NewAllocation(alloc.NodeID, ask)
+ newAlloc.uuid = alloc.UUID
+ return newAlloc
Review Comment:
yes, correct.
During recovery, application#addAllocationInternal populates `allocations`
map with allocation id/uuid. While doing this, entry overrides if key exists.
It happens while running `TestSchedulerRecovery` in
`pkg/scheduler/tests/recovery_test.go`. If we don't have this change, this test
fails. Test has 4 allocations alloc-1-0, alloc-1-1, alloc-2-0 & alloc-2-1.
After restart, allocations alloc-1-0 & alloc-2-0 change to alloc-1-1 &
alloc-2-1. So when the actual allocations with the same name later, it
overrides. Hence test fails.
--
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-unsubscr...@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [YUNIKORN-2204] Use ask unique id for allocation [yunikorn-core]
manirajv06 commented on code in PR #740:
URL: https://github.com/apache/yunikorn-core/pull/740#discussion_r1409083589
##
pkg/scheduler/objects/allocation.go:
##
@@ -152,7 +154,9 @@ func NewAllocationFromSI(alloc *si.Allocation) *Allocation {
createTime:time.Unix(creationTime, 0),
allocLog: make(map[string]*AllocationLogEntry),
}
- return NewAllocation(alloc.UUID, alloc.NodeID, ask)
+ newAlloc := NewAllocation(alloc.NodeID, ask)
+ newAlloc.uuid = alloc.UUID
+ return newAlloc
Review Comment:
yes, correct.
During recovery, application#addAllocationInternal populates `allocations`
map with allocation id/uuid. While doing this, entry overrides if key exists.
It happens while running `TestSchedulerRecovery` in
`pkg/scheduler/tests/recovery_test.go`. If we don't have this change, this test
fails.
--
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-unsubscr...@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [YUNIKORN-2204] Use ask unique id for allocation [yunikorn-core]
wilfred-s commented on code in PR #740:
URL: https://github.com/apache/yunikorn-core/pull/740#discussion_r1408672388
##
pkg/scheduler/tests/recovery_test.go:
##
Review Comment:
unneeded change for this jira
##
pkg/scheduler/objects/allocation.go:
##
@@ -295,6 +299,12 @@ func (a *Allocation) GetUUID() string {
return a.uuid
}
+// SetUUID set the uuid for this allocation
+// only for tests
+func (a *Allocation) SetUUID(uuid string) {
Review Comment:
Rename to `SetAllocationID` and the parameter rename to `id`.
Move away from the UUID as mentioned by Peter
##
pkg/scheduler/objects/allocation.go:
##
@@ -92,7 +92,7 @@ func NewAllocation(uuid, nodeID string, ask *AllocationAsk)
*Allocation {
bindTime: time.Now(),
nodeID:nodeID,
partitionName:
common.GetPartitionNameWithoutClusterID(ask.GetPartitionName()),
- uuid: uuid,
+ uuid: ask.allocationKey + "-" +
strconv.Itoa(ask.completedPendingAsk()),
Review Comment:
As a preparation add a new `GetAllocationID` function with the same
functionality as the `GetUUID` function in this change. Start calling that
instead of introducing 60+ new calls to `GetUUID` to simplify and reduce churn
in the follow up.
##
pkg/scheduler/objects/allocation.go:
##
@@ -152,7 +154,9 @@ func NewAllocationFromSI(alloc *si.Allocation) *Allocation {
createTime:time.Unix(creationTime, 0),
allocLog: make(map[string]*AllocationLogEntry),
}
- return NewAllocation(alloc.UUID, alloc.NodeID, ask)
+ newAlloc := NewAllocation(alloc.NodeID, ask)
+ newAlloc.uuid = alloc.UUID
+ return newAlloc
Review Comment:
Not sure that this is needed. Recovered allocation were already using the
pod UID. The only difference would be that a recovered allocation would become
-1 instead of -0 for normal allocations.
I do not see the harm in that. There will be a difference anyway as now we
just have without the trailing number.
##
pkg/scheduler/tests/mock_rm_callback_test.go:
##
Review Comment:
unneeded change for this jira
--
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-unsubscr...@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [YUNIKORN-2204] Use ask unique id for allocation [yunikorn-core]
wilfred-s commented on code in PR #740:
URL: https://github.com/apache/yunikorn-core/pull/740#discussion_r1408653359
##
pkg/scheduler/objects/allocation.go:
##
@@ -92,7 +92,7 @@ func NewAllocation(uuid, nodeID string, ask *AllocationAsk)
*Allocation {
bindTime: time.Now(),
nodeID:nodeID,
partitionName:
common.GetPartitionNameWithoutClusterID(ask.GetPartitionName()),
- uuid: uuid,
+ uuid: ask.allocationKey + "-" +
strconv.Itoa(ask.completedPendingAsk()),
Review Comment:
I would propose to rename in a follow up change:
* replace uuid with `allocationID` or `allocID`
* replace `GetUUID` with `GetAllocationID`
--
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-unsubscr...@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [YUNIKORN-2204] Use ask unique id for allocation [yunikorn-core]
pbacsko commented on code in PR #740:
URL: https://github.com/apache/yunikorn-core/pull/740#discussion_r1407983468
##
pkg/scheduler/objects/allocation.go:
##
@@ -92,7 +92,7 @@ func NewAllocation(uuid, nodeID string, ask *AllocationAsk)
*Allocation {
bindTime: time.Now(),
nodeID:nodeID,
partitionName:
common.GetPartitionNameWithoutClusterID(ask.GetPartitionName()),
- uuid: uuid,
+ uuid: ask.allocationKey + "-" +
strconv.Itoa(ask.completedPendingAsk()),
Review Comment:
Nit: it's no longer an "uuid". If renaming generates too much change, then
just add a short comment that "it's no longer uuid, but the variable name is
kept" or sth similar.
--
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-unsubscr...@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [YUNIKORN-2204] Use ask unique id for allocation [yunikorn-core]
codecov[bot] commented on PR #740: URL: https://github.com/apache/yunikorn-core/pull/740#issuecomment-1829990676 ## [Codecov](https://app.codecov.io/gh/apache/yunikorn-core/pull/740?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report Attention: `4 lines` in your changes are missing coverage. Please review. > Comparison is base [(`7d220b5`)](https://app.codecov.io/gh/apache/yunikorn-core/commit/7d220b519e7476c0ae17a348b54029060858346e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 77.82% compared to head [(`c13bd86`)](https://app.codecov.io/gh/apache/yunikorn-core/pull/740?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 77.94%. > Report is 11 commits behind head on master. | [Files](https://app.codecov.io/gh/apache/yunikorn-core/pull/740?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines | |---|---|---| | [pkg/scheduler/objects/allocation.go](https://app.codecov.io/gh/apache/yunikorn-core/pull/740?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | 81.81% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/yunikorn-core/pull/740?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [pkg/scheduler/objects/application.go](https://app.codecov.io/gh/apache/yunikorn-core/pull/740?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/yunikorn-core/pull/740?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Additional details and impacted files ```diff @@Coverage Diff @@ ## master #740 +/- ## == + Coverage 77.82% 77.94% +0.11% == Files 80 82 +2 Lines 1339913380 -19 == + Hits1042810429 +1 + Misses 2647 2628 -19 + Partials 324 323 -1 ``` [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/yunikorn-core/pull/740?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). -- 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-unsubscr...@yunikorn.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
