hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Wangda Tan (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-7135) Clean up lock-try order in common scheduler code
Date Tue, 19 Sep 2017 17:40:00 GMT

    [ 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\],
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

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

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

To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org

View raw message