[
https://issues.apache.org/jira/browse/YARN-7135?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16172065#comment-16172065
]
Wangda Tan commented on YARN-7135:
----------------------------------
[~v123582] / [~templedf],
Apologize for my late responses, I just checked some resources.
>From \[1\],
{code}
Assuming that lock is a ReentrantLock, then it makes no real difference, since
lock() does not throw any checked exceptions.
The Java documentation, however, leaves lock() outside the try block in the
ReentrantLock example. The reason for this is that an unchecked exception in
lock() should not lead to unlock() incorrectly being called. Whether
correctness is a concern in the presence of an unchecked exception in lock() of
all things, that is another discussion altogether.
It is a good coding practice in general to keep things like try blocks as
fine-grained as possible.
{code}
And you can also check that, Java's ReentrantLock.lock doesn't throw any
exception, see \[2\].
I think update all ReentrantLock in YARN RM package might be overkill and will
potentially cause lots of conflict when we want to do backport (unless we
backport this patch to all active branches).
Instead of doing this, I suggest to keep all ReentrantLock as-is, and only
update lock pattern when necessary.
\[1\] https://stackoverflow.com/questions/10868423/lock-lock-before-try
\[2\]
http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/locks/ReentrantLock.html
> Clean up lock-try order in common scheduler code
> ------------------------------------------------
>
> Key: YARN-7135
> URL: https://issues.apache.org/jira/browse/YARN-7135
> Project: Hadoop YARN
> Issue Type: Improvement
> Components: scheduler
> Affects Versions: 3.0.0-alpha4
> Reporter: Daniel Templeton
> Assignee: weiyuan
> Labels: newbie
> Attachments: YARN-7135.001.patch, YARN-7135.002.patch,
> YARN-7135.003.patch
>
>
> There are many places that follow the pattern:{code}try {
> lock.lock();
> ...
> } finally {
> lock.unlock();
> }{code}
> There are a couple of reasons that's a bad idea. The correct pattern
> is:{code}lock.lock();
> try {
> ...
> } finally {
> lock.unlock();
> }{code}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
