wilfred-s commented on code in PR #950:
URL: https://github.com/apache/yunikorn-core/pull/950#discussion_r1721378063
##########
pkg/scheduler/objects/application.go:
##########
@@ -1660,6 +1664,8 @@ func (sa *Application) addAllocationInternal(allocType
AllocationResultType, all
sa.incUserResourceUsage(alloc.GetAllocatedResource())
sa.allocatedPlaceholder =
resources.Add(sa.allocatedPlaceholder, alloc.GetAllocatedResource())
sa.maxAllocatedResource =
resources.ComponentWiseMax(sa.allocatedPlaceholder, sa.maxAllocatedResource)
+ sa.allocatedPlaceholder.Prune()
+ sa.maxAllocatedResource.Prune()
Review Comment:
i would be really surprised if that would ever have a zero in any type. We
can only add a type to this aggregate if it is in the request of an allocation
as a non zero value. If we prune `allocatedPlaceholder` before we pass it into
the calculation we only need one prune call.
move `allocatedPlaceholder.Prune()` upone line and remove
`maxAllocatedResource.Prune()`
##########
pkg/scheduler/objects/application.go:
##########
@@ -748,6 +751,7 @@ func (sa *Application) deallocateAsk(ask *Allocation)
(*resources.Resource, erro
delta := ask.GetAllocatedResource()
sa.pending = resources.Add(sa.pending, delta)
+ sa.pending.Prune()
Review Comment:
ask or alloc should not have zero types, no need to prune
The delta that gets returned is also dropped by all callers or expected to
be nil. Except for 1 tests which does not really need it. Not sure we should
keep it.
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1609,20 +1615,36 @@ func (sq *Queue) updateMaxResourceMetrics() {
// updateAllocatedResourceMetrics updates allocated resource metrics for all
queue types.
func (sq *Queue) updateAllocatedResourceMetrics() {
+ // Special case when we prune the allocated resources to zero, we need
to update the metrics
+ // here only keep basic types memory and vcores to make sure we have
updated metrics
+ if sq.allocatedResource == nil || len(sq.allocatedResource.Resources)
== 0 {
+
metrics.GetQueueMetrics(sq.QueuePath).SetQueueAllocatedResourceMetrics("memory",
float64(0))
+
metrics.GetQueueMetrics(sq.QueuePath).SetQueueAllocatedResourceMetrics("vcores",
float64(0))
+ }
for k, v := range sq.allocatedResource.Resources {
metrics.GetQueueMetrics(sq.QueuePath).SetQueueAllocatedResourceMetrics(k,
float64(v))
}
}
// updatePendingResourceMetrics updates pending resource metrics for all queue
types.
func (sq *Queue) updatePendingResourceMetrics() {
+ // Special case when we prune the allocated resources to zero, we need
to update the metrics
+ // here only keep basic types memory and vcores to make sure we have
updated metrics
+ if sq.pending == nil || len(sq.pending.Resources) == 0 {
Review Comment:
use `sq.pending.IsZero()`
##########
pkg/scheduler/objects/node.go:
##########
@@ -377,6 +382,8 @@ func (sn *Node) ReplaceAllocation(allocationKey string,
replace *Allocation, del
// The allocatedResource and availableResource should be updated in the
same way
sn.allocatedResource.AddTo(delta)
sn.availableResource.SubFrom(delta)
+ sn.allocatedResource.Prune()
+ sn.availableResource.Prune()
Review Comment:
The alloc should not have zeros in it. The only prune that is needed is when
we do the `SubFrom`
##########
pkg/scheduler/objects/application.go:
##########
@@ -1660,6 +1664,8 @@ func (sa *Application) addAllocationInternal(allocType
AllocationResultType, all
sa.incUserResourceUsage(alloc.GetAllocatedResource())
sa.allocatedPlaceholder =
resources.Add(sa.allocatedPlaceholder, alloc.GetAllocatedResource())
sa.maxAllocatedResource =
resources.ComponentWiseMax(sa.allocatedPlaceholder, sa.maxAllocatedResource)
+ sa.allocatedPlaceholder.Prune()
+ sa.maxAllocatedResource.Prune()
Review Comment:
On top of that alloc should never have zero values in it so not sure we need
to prune at all here.
##########
pkg/scheduler/objects/node.go:
##########
@@ -312,6 +313,8 @@ func (sn *Node) RemoveAllocation(allocationKey string)
*Allocation {
delete(sn.allocations, allocationKey)
sn.allocatedResource.SubFrom(alloc.GetAllocatedResource())
sn.availableResource.AddTo(alloc.GetAllocatedResource())
+ sn.allocatedResource.Prune()
+ sn.availableResource.Prune()
Review Comment:
The alloc should not have zeros in it. The only prune that is needed is when
we do the `SubFrom`
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1609,20 +1615,36 @@ func (sq *Queue) updateMaxResourceMetrics() {
// updateAllocatedResourceMetrics updates allocated resource metrics for all
queue types.
func (sq *Queue) updateAllocatedResourceMetrics() {
+ // Special case when we prune the allocated resources to zero, we need
to update the metrics
+ // here only keep basic types memory and vcores to make sure we have
updated metrics
+ if sq.allocatedResource == nil || len(sq.allocatedResource.Resources)
== 0 {
Review Comment:
use `sq.allocatedResource.IsZero()`
##########
pkg/scheduler/ugm/queue_tracker.go:
##########
@@ -113,6 +113,7 @@ func (qt *QueueTracker) increaseTrackedResource(hierarchy
[]string, applicationI
qt.resourceUsage = resources.NewResource()
}
qt.resourceUsage.AddTo(usage)
+ qt.resourceUsage.Prune()
Review Comment:
The alloc should not have zeros in it. The only prune that is needed is when
we do the `Sub` or `SubFrom`
##########
pkg/scheduler/objects/application.go:
##########
@@ -728,6 +730,7 @@ func (sa *Application) allocateAsk(ask *Allocation)
(*resources.Resource, error)
delta := resources.Multiply(ask.GetAllocatedResource(), -1)
sa.pending = resources.Add(sa.pending, delta)
+ sa.pending.Prune()
// update the pending of the queue with the same delta
sa.queue.incPendingResource(delta)
Review Comment:
This construct looks weird: multiply by -1 and then call Add. We should just
call `Sub` and `decPendingResource`.
The delta that gets returned is also dropped by all callers or expected to
be nil. Except for 1 tests which does not really need it. Not sure we should
keep it.
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1609,20 +1615,36 @@ func (sq *Queue) updateMaxResourceMetrics() {
// updateAllocatedResourceMetrics updates allocated resource metrics for all
queue types.
func (sq *Queue) updateAllocatedResourceMetrics() {
+ // Special case when we prune the allocated resources to zero, we need
to update the metrics
+ // here only keep basic types memory and vcores to make sure we have
updated metrics
+ if sq.allocatedResource == nil || len(sq.allocatedResource.Resources)
== 0 {
+
metrics.GetQueueMetrics(sq.QueuePath).SetQueueAllocatedResourceMetrics("memory",
float64(0))
+
metrics.GetQueueMetrics(sq.QueuePath).SetQueueAllocatedResourceMetrics("vcores",
float64(0))
+ }
for k, v := range sq.allocatedResource.Resources {
metrics.GetQueueMetrics(sq.QueuePath).SetQueueAllocatedResourceMetrics(k,
float64(v))
}
}
// updatePendingResourceMetrics updates pending resource metrics for all queue
types.
func (sq *Queue) updatePendingResourceMetrics() {
+ // Special case when we prune the allocated resources to zero, we need
to update the metrics
+ // here only keep basic types memory and vcores to make sure we have
updated metrics
+ if sq.pending == nil || len(sq.pending.Resources) == 0 {
+
metrics.GetQueueMetrics(sq.QueuePath).SetQueuePendingResourceMetrics("memory",
float64(0))
+
metrics.GetQueueMetrics(sq.QueuePath).SetQueuePendingResourceMetrics("vcores",
float64(0))
+ }
for k, v := range sq.pending.Resources {
metrics.GetQueueMetrics(sq.QueuePath).SetQueuePendingResourceMetrics(k,
float64(v))
}
}
// updatePreemptingResourceMetrics updates preempting resource metrics for all
queue types.
func (sq *Queue) updatePreemptingResourceMetrics() {
+ if sq.pending == nil || len(sq.pending.Resources) == 0 {
Review Comment:
use `sq.pending.IsZero()`
##########
pkg/scheduler/objects/application.go:
##########
@@ -1686,6 +1692,8 @@ func (sa *Application) addAllocationInternal(allocType
AllocationResultType, all
sa.incUserResourceUsage(alloc.GetAllocatedResource())
sa.allocatedResource = resources.Add(sa.allocatedResource,
alloc.GetAllocatedResource())
sa.maxAllocatedResource =
resources.ComponentWiseMax(sa.allocatedResource, sa.maxAllocatedResource)
+ sa.allocatedResource.Prune()
+ sa.maxAllocatedResource.Prune()
Review Comment:
Same as above
##########
pkg/scheduler/objects/node.go:
##########
@@ -353,6 +356,8 @@ func (sn *Node) addAllocationInternal(alloc *Allocation,
force bool) bool {
sn.allocations[alloc.GetAllocationKey()] = alloc
sn.allocatedResource.AddTo(res)
sn.availableResource.SubFrom(res)
+ sn.allocatedResource.Prune()
+ sn.availableResource.Prune()
Review Comment:
The alloc should not have zeros in it. The only prune that is needed is when
we do the `SubFrom`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
