[
https://issues.apache.org/jira/browse/YUNIKORN-2209?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17791947#comment-17791947
]
Peter Bacsko edited comment on YUNIKORN-2209 at 12/1/23 8:27 AM:
-----------------------------------------------------------------
Thanks [~wilfreds] yes I found the code which actually does check the return
value.
Based on our offline discussion, we need some cleanup here, because it's
difficult to understand what's happening. The logic is hard to follow.
My observations regarding {{UserTracker}}:
* {{increasedTrackedResource()}}: The first boolean "increased" should always
be true, otherwise, it won't be undone
* {{increasedTrackedResource()}}: The lookup
"ut.appGroupTrackers[applicationID]" should be checked against the missing
case. That's a bug, but let's log it properly and don't crash a nil pointer.
* The comment above "appGroupTrackers" is difficult to interpret.
I understand that if the limit is hit, we don't want to increase resource usage
for a particular group. However, it still looks fishy, we undo the calculation
for the userTracker, not for the appGroupTracker. So when the allocation is
done, we'll done another decrement... Which makes me think why we need a
decrement at all? Again, it probably works as intended, but I just can't see
why.
was (Author: pbacsko):
Thanks [~wilfreds] yes I found the code which actually does check the return
value.
Based on our offline discussion, we need some cleanup here, because it's
difficult to understand what's happening. The logic is hard to follow.
My observations regarding {{UserTracker}}:
* {{increasedTrackedResource()}}: The first boolean "increased" should always
be true, otherwise, it won't be undone
* {{increasedTrackedResource()}}: The lookup
"ut.appGroupTrackers[applicationID]" should be checked against the missing
case. That's a bug, but let's log it properly and don't crash a nil pointer.
* The comment above "appGroupTrackers" is difficult to interpret.
I understand that if the limit is hit, we don't want to increase resource usage
for a particular group. However, it still looks fishy, we undo the calculation
for the userTracker, not for the appGroupTracker. So when the allocation is
done, we'll done another decrement... Which makes me think why we need a
decrement at all? Again, it probably works as intended, but difficult to see
why.
> Remove limit checks in QueueTracker
> -----------------------------------
>
> Key: YUNIKORN-2209
> URL: https://issues.apache.org/jira/browse/YUNIKORN-2209
> Project: Apache YuniKorn
> Issue Type: Sub-task
> Components: core - common
> Reporter: Peter Bacsko
> Assignee: Peter Bacsko
> Priority: Major
>
> {{QueueTracker.increaseTrackedResource()}} contains code that is no longer
> relevant and is a good candidate for removal.
> It verifies whether the increased resource is over certain limits. However,
> this is not the responsibility of the tracker, at least not anymore. The
> method returns a boolean which is no longer used by the application.
> Worse, we ignore the increment calculation but perform the decrement part.
> This results in a corrupted state. Even if we detect that limits are
> violated, there's no reason to mess things up even further.
> It also has performance impacts. Lot of intermediate Resource objects are
> created, eg. "finalResourceUsage", {{resources.NewResource()}} is called
> multiple times. These all results in heap allocations and they immediately
> become garbage as soon as the method returns. Actually after performing
> YUNIKORN-2201, {{Manager.IncreaseTrackedResource()}} is a 1.5-2% contributor
> to the overall heap and cpu usage. Not a massive save, but if it's easy to
> gain a quick improvement, let's go for it.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
