[
https://issues.apache.org/jira/browse/YARN-2925?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14236461#comment-14236461
]
Craig Welch commented on YARN-2925:
-----------------------------------
So, a few things things:
I can see how we could get stale values for usedResources during the
getHeadroom call - the usedResources component of headroom calculation was
added after the initial change to do final computation on demand and the
potential impact of that not thought through all the way, clearly. A comment
there to the effect that there is an additional entry point thread wise to
consider would have helped (& I think we should add it with this change for the
future...).
There is synchronization on each active application during computeUserLimit*,
and updates to usedResources occur there, and the getHeadroom call is also
synchronized on the application, so there is a high degree of likelihood of
reasonable consistency in practice, but no guarantee as far as I can see.
Fortunately, the low level value fields of Resource are "ints", so the worst
that can happen is that we are working with stale values (not "values which
were never set/random int values"...), but we don't want to be working with
stale values as that will mean we could get incorrect results on occasion.
At the moment the consistent lock point between the allocate() where headroom
is retrieved and the compute... in the queue is only the application, which is
not sufficient to cover those some items which are further shared -
usedResource (which you were looking at), the user's resource (consumedResource
on the User object associated with the application) and clusterResource (which
need to be dealt with too as far as I can see - though it is probably rarely an
issue in practice, should fix it now too).
All that said, some thoughts on the particulars of your patch:
1. I think we should combine the QueueHeadroomInfo and the QueueResourceInfo
types - I didn't get to it before you posted your patch, but the purpose of
QueueHeadroomInfo was already to provide a finer grained lock point for the
very reasons outlined here, and it is doing so for certain items, but it
doesn't catch them all or do it in a way which is 100% safe as you've noted. I
don't think it matters which one we end up but we don't need both and they are
both trying to accomplish the same thing, so we should combine them.
2. I don't think the patch as it stands will resolve the issue as it is just
asserting consistency for access to the resource members of QueueResourceInfo -
the problem is the calculation of headroom which makes use of their values
which could occur concurrently with the addTo or subtractFrom operations after
the references have been pulled from the accessor. The lock consistency will
need to also encompass the calculation which occurs in getHeadroom(User user,
Resource queueMaxCap,
Resource clusterResource, Resource userLimit) in LeafQueue to be
successful
Also, "usedResources" does not appear to be used in either AbstractCSQueue or
ParentQueue for anything except debug logging - and as applications can only
run in leaf queues, I believe it should be relocated back to be a member of
LeafQueue. Given that, I think that the QueueHeadroomXYZ class should live in
the LeafQueue for simplicity/encapsulation. Finally, the clusterResource also
needs to be handled here, as well as the user's consumed resource
(user.getTotalConsumedResources())
I also don't think that the read/write lock complexity is called for here, I
don't think it's helping anything and will make the code/understanding the code
more complex, we should just synchronize on the QueueXYZInfo instance which
survives the combination of QHeadroomInfo and QResourceInfo
Given all that, I think the full fix would be:
Combine QHeadroom* and QueueResource* into one class who's per-queue- instance
lives in LeafQueue
relocate UsedResources toLeafQueue (in the QHead/Resource* instance)
move the final getHeadroom calculation into that same class, synchronize it.
move the add/subtract to/from for usedResources into a synchronized method of
that class (much as you have), and do the same for the user consumed resource
values (and in a sense cluster, see below)
clusterResource -
I think this one also needs to be handled - the instance of this which is used
should be copied when it is set inside a synchronized method of the
QHeadroom|Resource* class which we end up with - setting of the clusterResource
appears to happen consistently when it changes (via updateClusterResource so it
should be kept sufficiently up to date with the cluster resource and by copying
it and then we don't have to try to lock the scheduler which is where it is be
modified and which would be too intrusive.
[~leftnoteasy] does this make sense?
> Internal fields in LeafQueue access should be protected when accessed from
> FiCaSchedulerApp to calculate Headroom
> -----------------------------------------------------------------------------------------------------------------
>
> Key: YARN-2925
> URL: https://issues.apache.org/jira/browse/YARN-2925
> Project: Hadoop YARN
> Issue Type: Bug
> Components: capacityscheduler
> Reporter: Wangda Tan
> Assignee: Wangda Tan
> Priority: Critical
> Attachments: YARN-2925.1.patch
>
>
> Upon YARN-2644, FiCaScheduler will calculation up-to-date headroom before
> sending back Allocation response to AM.
> Headroom calculation is happened in LeafQueue side, uses fields like used
> resource, etc. But it is not protected by any lock of LeafQueue, so it might
> be corrupted is someone else is editing it.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
