[jira] [Commented] (YARN-9341) Reentrant lock() before try
[
https://issues.apache.org/jira/browse/YARN-9341?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16787432#comment-16787432
]
Prabhu Joseph commented on YARN-9341:
-
Thanks [~eyang] and [~wilfreds]!
> Reentrant lock() before try
> ---
>
> Key: YARN-9341
> URL: https://issues.apache.org/jira/browse/YARN-9341
> Project: Hadoop YARN
> Issue Type: Bug
> Components: yarn
>Affects Versions: 3.1.0
>Reporter: Prabhu Joseph
>Assignee: Prabhu Joseph
>Priority: Minor
> Fix For: 3.3.0
>
> Attachments: YARN-9341-001.patch
>
>
> As a best practice - Reentrant lock has to be acquired before try clause.
> https://stackoverflow.com/questions/31058681/java-locking-structure-best-pattern
> There are many places where lock is obtained inside try.
> {code}
> try {
>this.writeLock.lock();
>
> } finally {
> this.writeLock.unlock();
> }
> {code}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
-
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-9341) Reentrant lock() before try
[
https://issues.apache.org/jira/browse/YARN-9341?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16787232#comment-16787232
]
Eric Yang commented on YARN-9341:
-
[~wilfreds] You are right. They have been handled correctly. Thank you for
the review.
+1
> Reentrant lock() before try
> ---
>
> Key: YARN-9341
> URL: https://issues.apache.org/jira/browse/YARN-9341
> Project: Hadoop YARN
> Issue Type: Bug
> Components: yarn
>Affects Versions: 3.1.0
>Reporter: Prabhu Joseph
>Assignee: Prabhu Joseph
>Priority: Minor
> Attachments: YARN-9341-001.patch
>
>
> As a best practice - Reentrant lock has to be acquired before try clause.
> https://stackoverflow.com/questions/31058681/java-locking-structure-best-pattern
> There are many places where lock is obtained inside try.
> {code}
> try {
>this.writeLock.lock();
>
> } finally {
> this.writeLock.unlock();
> }
> {code}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
-
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-9341) Reentrant lock() before try
[ https://issues.apache.org/jira/browse/YARN-9341?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16787239#comment-16787239 ] Hudson commented on YARN-9341: -- FAILURE: Integrated in Jenkins build Hadoop-trunk-Commit #16157 (See [https://builds.apache.org/job/Hadoop-trunk-Commit/16157/]) YARN-9341. Fixed enentrant lock usage in YARN project. (eyang: rev 39b4a37e02e929a698fcf9e32f1f71bb6b977635) * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/monitor/capacity/IntraQueueCandidatesSelector.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/constraint/MemoryPlacementConstraintManager.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/ManagedParentQueue.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/AppSchedulingInfo.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/LeafQueue.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractCSQueue.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/PlacementManager.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacityScheduler.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/AbstractYarnScheduler.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/RMAppImpl.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/nodelabels/NodeAttributesManagerImpl.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/LocalizedResource.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/volume/csi/lifecycle/VolumeImpl.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/rmcontainer/RMContainerImpl.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/AbstractResourceUsage.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/application/ApplicationImpl.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/attempt/RMAppAttemptImpl.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/client/api/impl/FileSystemTimelineWriter.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services/hadoop-yarn-services-core/src/main/java/org/apache/hadoop/yarn/service/ServiceManager.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/ParentQueue.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/queuemanagement/GuaranteedOrZeroCapacityOverTimePolicy.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/main/java/org/apache/hadoop/yarn/server/timeline/LeveldbTimelineStore.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/ReservationQueue.java * (edit)
[jira] [Commented] (YARN-9341) Reentrant lock() before try
[
https://issues.apache.org/jira/browse/YARN-9341?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16785162#comment-16785162
]
Wilfred Spiegelenburg commented on YARN-9341:
-
So we do have one and also one {{lockInterruptibly()}} in another part. The
change as proposed by [~Prabhu Joseph] have left those two unchanged, neither
are covered under the description of the jira either. It just talks about the
{{lock()}} cases.
The only replacements that have been made are the direct calls to {{lock()}} in
the patch neither of the two other ones have been touched. That is where I
based my +1 on
> Reentrant lock() before try
> ---
>
> Key: YARN-9341
> URL: https://issues.apache.org/jira/browse/YARN-9341
> Project: Hadoop YARN
> Issue Type: Bug
> Components: yarn
>Affects Versions: 3.1.0
>Reporter: Prabhu Joseph
>Assignee: Prabhu Joseph
>Priority: Minor
> Attachments: YARN-9341-001.patch
>
>
> As a best practice - Reentrant lock has to be acquired before try clause.
> https://stackoverflow.com/questions/31058681/java-locking-structure-best-pattern
> There are many places where lock is obtained inside try.
> {code}
> try {
>this.writeLock.lock();
>
> } finally {
> this.writeLock.unlock();
> }
> {code}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
-
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-9341) Reentrant lock() before try
[
https://issues.apache.org/jira/browse/YARN-9341?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16784984#comment-16784984
]
Eric Yang commented on YARN-9341:
-
hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/logaggregation/TestLogAggregationService.java
uses tryLock. The test case might need to be handled with more care.
> Reentrant lock() before try
> ---
>
> Key: YARN-9341
> URL: https://issues.apache.org/jira/browse/YARN-9341
> Project: Hadoop YARN
> Issue Type: Bug
> Components: yarn
>Affects Versions: 3.1.0
>Reporter: Prabhu Joseph
>Assignee: Prabhu Joseph
>Priority: Minor
> Attachments: YARN-9341-001.patch
>
>
> As a best practice - Reentrant lock has to be acquired before try clause.
> https://stackoverflow.com/questions/31058681/java-locking-structure-best-pattern
> There are many places where lock is obtained inside try.
> {code}
> try {
>this.writeLock.lock();
>
> } finally {
> this.writeLock.unlock();
> }
> {code}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
-
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-9341) Reentrant lock() before try
[
https://issues.apache.org/jira/browse/YARN-9341?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16783987#comment-16783987
]
Wilfred Spiegelenburg commented on YARN-9341:
-
An {{IllegalMonitorStateException}} can only happen on unlock if the current
thread is not the owner of the lock.
I don't think we use {{tryLock}} or {{lockInterruptibly}} anywhere in our code
and thus do not need to worry about the {{IllegalMonitorStateException}}. When
you call lock the thread is blocked until the point you acquire the lock. We
should thus never proceed beyond the lock line and the finally clause should
never be executed until after the thread has lock.
The change proposed is even following the java [API
doc|https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/locks/ReentrantLock.html]
for the locking.
+1 (non binding)
> Reentrant lock() before try
> ---
>
> Key: YARN-9341
> URL: https://issues.apache.org/jira/browse/YARN-9341
> Project: Hadoop YARN
> Issue Type: Bug
> Components: yarn
>Affects Versions: 3.1.0
>Reporter: Prabhu Joseph
>Assignee: Prabhu Joseph
>Priority: Minor
> Attachments: YARN-9341-001.patch
>
>
> As a best practice - Reentrant lock has to be acquired before try clause.
> https://stackoverflow.com/questions/31058681/java-locking-structure-best-pattern
> There are many places where lock is obtained inside try.
> {code}
> try {
>this.writeLock.lock();
>
> } finally {
> this.writeLock.unlock();
> }
> {code}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
-
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-9341) Reentrant lock() before try
[
https://issues.apache.org/jira/browse/YARN-9341?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16783671#comment-16783671
]
Eric Yang commented on YARN-9341:
-
[~Prabhu Joseph] When lock is moved outside of try, unlock may throw
IllegalMonitorStateException, which is a RuntimeException. The caller must
handle RuntimeException correctly otherwise some threads may stop working when
unable to lock.
> Reentrant lock() before try
> ---
>
> Key: YARN-9341
> URL: https://issues.apache.org/jira/browse/YARN-9341
> Project: Hadoop YARN
> Issue Type: Bug
> Components: yarn
>Affects Versions: 3.1.0
>Reporter: Prabhu Joseph
>Assignee: Prabhu Joseph
>Priority: Minor
> Attachments: YARN-9341-001.patch
>
>
> As a best practice - Reentrant lock has to be acquired before try clause.
> https://stackoverflow.com/questions/31058681/java-locking-structure-best-pattern
> There are many places where lock is obtained inside try.
> {code}
> try {
>this.writeLock.lock();
>
> } finally {
> this.writeLock.unlock();
> }
> {code}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
-
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-9341) Reentrant lock() before try
[
https://issues.apache.org/jira/browse/YARN-9341?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16783366#comment-16783366
]
Prabhu Joseph commented on YARN-9341:
-
[~ajisakaa] Can you review this jira. The patch failures are not related and
handled in YARN-9338.
> Reentrant lock() before try
> ---
>
> Key: YARN-9341
> URL: https://issues.apache.org/jira/browse/YARN-9341
> Project: Hadoop YARN
> Issue Type: Bug
> Components: yarn
>Affects Versions: 3.1.0
>Reporter: Prabhu Joseph
>Assignee: Prabhu Joseph
>Priority: Minor
> Attachments: YARN-9341-001.patch
>
>
> As a best practice - Reentrant lock has to be acquired before try clause.
> https://stackoverflow.com/questions/31058681/java-locking-structure-best-pattern
> There are many places where lock is obtained inside try.
> {code}
> try {
>this.writeLock.lock();
>
> } finally {
> this.writeLock.unlock();
> }
> {code}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
-
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-9341) Reentrant lock() before try
[
https://issues.apache.org/jira/browse/YARN-9341?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16783268#comment-16783268
]
Hadoop QA commented on YARN-9341:
-
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
20s{color} | {color:blue} Docker mode activated. {color} |
|| || || || {color:brown} Prechecks {color} ||
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 1 new or modified test
files. {color} |
|| || || || {color:brown} trunk Compile Tests {color} ||
| {color:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 0m
14s{color} | {color:blue} Maven dependency ordering for branch {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 17m
24s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 10m
32s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
57s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 4m
37s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
19m 3s{color} | {color:green} branch has no errors when building and testing
our client artifacts. {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 5m
37s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 2m
54s{color} | {color:green} trunk passed {color} |
|| || || || {color:brown} Patch Compile Tests {color} ||
| {color:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 0m
14s{color} | {color:blue} Maven dependency ordering for patch {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 2m
55s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 8m
18s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 8m
18s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
45s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 3m
34s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green} The patch has no whitespace issues. {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
12m 25s{color} | {color:green} patch has no errors when building and testing
our client artifacts. {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 6m
8s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 2m
44s{color} | {color:green} the patch passed {color} |
|| || || || {color:brown} Other Tests {color} ||
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 3m
43s{color} | {color:green} hadoop-yarn-common in the patch passed. {color} |
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 21m
21s{color} | {color:green} hadoop-yarn-server-nodemanager in the patch passed.
{color} |
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 4m
7s{color} | {color:green} hadoop-yarn-server-applicationhistoryservice in the
patch passed. {color} |
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 82m 56s{color}
| {color:red} hadoop-yarn-server-resourcemanager in the patch failed. {color} |
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 18m
1s{color} | {color:green} hadoop-yarn-services-core in the patch passed.
{color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
36s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black}227m 32s{color} |
{color:black} {color} |
\\
\\
|| Reason || Tests ||
| Failed junit tests |
hadoop.yarn.server.resourcemanager.metrics.TestSystemMetricsPublisherForV2 |
| |
hadoop.yarn.server.resourcemanager.metrics.TestCombinedSystemMetricsPublisher |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f |
| JIRA Issue | YARN-9341 |
| JIRA Patch URL |
[jira] [Commented] (YARN-9341) Reentrant lock() before try
[
https://issues.apache.org/jira/browse/YARN-9341?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16783062#comment-16783062
]
Prabhu Joseph commented on YARN-9341:
-
Reviewed below files which are using lock() in hadoop-yarn-project.
{code}
HW12663:hadoop-yarn-project pjoseph$ find . -name *.java | xargs grep -w
"lock()" | wc -l
662
{code}
> Reentrant lock() before try
> ---
>
> Key: YARN-9341
> URL: https://issues.apache.org/jira/browse/YARN-9341
> Project: Hadoop YARN
> Issue Type: Bug
> Components: yarn
>Affects Versions: 3.1.0
>Reporter: Prabhu Joseph
>Assignee: Prabhu Joseph
>Priority: Minor
> Attachments: YARN-9341-001.patch
>
>
> As a best practice - Reentrant lock has to be acquired before try clause.
> https://stackoverflow.com/questions/31058681/java-locking-structure-best-pattern
> There are many places where lock is obtained inside try.
> {code}
> try {
>this.writeLock.lock();
>
> } finally {
> this.writeLock.unlock();
> }
> {code}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
-
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
