[
https://issues.apache.org/jira/browse/YARN-6025?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15779295#comment-15779295
]
Naganarasimha G R commented on YARN-6025:
-----------------------------------------
My bad. i wrongly read the code as CS holding its private lock (different from
the AbstractYarnScheduler) because of which i had given earlier few comments.
Now i realize many things clearly. Thanks to [~sunilg] (for offline chat) and
[~wangda] for clarifying it.
But just one more doubt.
FairScheduler holds write lock while doing {{super.nodeUpdate(nm)}} and CS
holds read lock, should it not be that we should hold a lock inside
{{AbstractYarnScheduler.nodeUpdate(RMNode)}} as
AbstractYarnScheduler.nodeUpdate is mainly sending out the events for the
container finished, updating the health status. In fact all the AYS.nodeUpdate
operations could be done even without any lock so multiple nodes could update
these states in parallel, but may be read lock is required so that write lock
will wait for all readlock operations to finish and then do operations under
it.
For the first point {{"removing the syncronization on nodeUpdate and extending
and keeping it in FifoScheduler"}} will update the patch.
> Few issues in synchronization in CapacityScheduler & AbstractYarnScheduler
> --------------------------------------------------------------------------
>
> Key: YARN-6025
> URL: https://issues.apache.org/jira/browse/YARN-6025
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: capacity scheduler, scheduler
> Reporter: Naganarasimha G R
> Assignee: Naganarasimha G R
> Attachments: YARN-6025.01.patch
>
>
> YARN-3139 does optimization on the locks by introducing
> ReentrantReadWriteLock to remove synchronized but seems to have some issues.
> # CapacityScheduler
> #* nodeUpdate(RMNode) need not be synchronized, as its the only one to be in
> the class
> #* setLastNodeUpdateTime in nodeUpdate needs to be updated with readLock ?
> then getLastNodeUpdateTime is done without any lock and more over its
> volatile.
> #* getUserGroupMappingPlacementRule need not be public as its held called
> within and not used in test and further is called from initScheduler and
> reinitialize where both are holding write locks so i presume getting read
> locks are of no use.
> # AbstractYarnScheduler
> #* recoverContainersOnNode is synchronized as well as holds write lock on the
> complete method so i presume we do not require synchronized here.
> #* nodeUpdate method too is synchronized but if i see the updates done inside
> i do not see any place where node update from two different nodes will have
> any issues (except for schedulerHealth which is taken care internally with
> concurrentHashMap), And even if require we could better use write lock. (also
> depends on the decision of next point)
> #* readLock is only used in containerLaunchedOnNode which i am not completely
> sure whether its required to have a read lock here, suppose we do not require
> then whether there is any use of read write locks in AbstractYarnScheduler as
> in general there is performance overhead in using readwrite lock over
> synchronized blocks on frequently accessed code path.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
