ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dmytro Shkvyra" <dshkv...@hortonworks.com>
Subject Re: Review Request 35074: ClusterDeadlockTest unit test fails
Date Mon, 08 Jun 2015 18:53:03 GMT


> On June 8, 2015, 4:57 p.m., John Speidel wrote:
> > ambari-server/src/test/java/org/apache/ambari/server/state/cluster/ClusterDeadlockTest.java,
line 181
> > <https://reviews.apache.org/r/35074/diff/2/?file=980026#file980026line181>
> >
> >     why do you throw exceptions here?  Why not just do assertions inline to be more
clear?

Agree


> On June 8, 2015, 4:57 p.m., John Speidel wrote:
> > ambari-server/src/test/java/org/apache/ambari/server/state/cluster/ClusterDeadlockTest.java,
line 214
> > <https://reviews.apache.org/r/35074/diff/2/?file=980026#file980026line214>
> >
> >     same comment as line 214

Agree


> On June 8, 2015, 4:57 p.m., John Speidel wrote:
> > ambari-server/src/test/java/org/apache/ambari/server/state/cluster/ClusterDeadlockTest.java,
line 266
> > <https://reviews.apache.org/r/35074/diff/2/?file=980026#file980026line266>
> >
> >     same comment as line 214

Agree


> On June 8, 2015, 4:57 p.m., John Speidel wrote:
> > ambari-server/src/test/java/org/apache/ambari/server/state/cluster/DeadlockWarningThread.java,
line 36
> > <https://reviews.apache.org/r/35074/diff/2/?file=980027#file980027line36>
> >
> >     Is this class meant to be used outside of tests?  If so, please describe it's
usage in ambari runtime.
> >     
> >     Many of my comments are based on the assumption that this class is to be used
only in tests.
> >     
> >     If the intention is for this class to be used in ambari runtime, I will provide
additional comments/issues.
> >     
> >     If this class is test only, please change the package.

Agree.


> On June 8, 2015, 4:57 p.m., John Speidel wrote:
> > ambari-server/src/test/java/org/apache/ambari/server/state/cluster/DeadlockWarningThread.java,
line 79
> > <https://reviews.apache.org/r/35074/diff/2/?file=980027#file980027line79>
> >
> >     why not findDeadlockedThreads()?

It is not work if deadlock caused by use ReadWriteLock


> On June 8, 2015, 4:57 p.m., John Speidel wrote:
> > ambari-server/src/test/java/org/apache/ambari/server/state/cluster/DeadlockWarningThread.java,
line 90
> > <https://reviews.apache.org/r/35074/diff/2/?file=980027#file980027line90>
> >
> >     why are there 2 different mechanisms for detecting deadlock here?
> >     
> >     mbean.findMonitorDeadlockedThreads() and then all of this code in the else block

It is not work if deadlock caused by use ReadWriteLock


> On June 8, 2015, 4:57 p.m., John Speidel wrote:
> > ambari-server/src/test/java/org/apache/ambari/server/state/cluster/DeadlockWarningThread.java,
line 93
> > <https://reviews.apache.org/r/35074/diff/2/?file=980027#file980027line93>
> >
> >     if the test is interrupted this really doesn't need to be logged IMO.  Also,
by catching InterruptedException and eating it, you are effectively preventing this thread
from being interrupted.  It would seem that you would want to exit the thread and preserve
the interrupted flag in this case

Agree


> On June 8, 2015, 4:57 p.m., John Speidel wrote:
> > ambari-server/src/test/java/org/apache/ambari/server/state/cluster/DeadlockWarningThread.java,
line 112
> > <https://reviews.apache.org/r/35074/diff/2/?file=980027#file980027line112>
> >
> >     see above comment questioning why we have 2 deadlock detection mechanisms.
> >     
> >     We do you need to check each active thread again to see if it is alive?

findMonitorDeadlockedThreads() can't detect these kind of deadlocks  http://java.dzone.com/articles/java-concurrency-hidden-thread,
so we need detect it in more common way. But if flat deadlock happened we should use more
habitual instrument in order get more clear info about deadlock.


> On June 8, 2015, 4:57 p.m., John Speidel wrote:
> > ambari-server/src/test/java/org/apache/ambari/server/state/cluster/DeadlockWarningThread.java,
line 121
> > <https://reviews.apache.org/r/35074/diff/2/?file=980027#file980027line121>
> >
> >     See earlier message questioning why we have 2 deadlock detection mechanisms
here.
> >     
> >     Comparing the current call stack against the last call stack to determine whether
a deadlock has occurred seems dangerous here and likely to result in occasional detected deadlocks
in cases where none exist.  For example if there is an extended GC during this test and none
of the monitored threads make progress but this thread does, a false deadlock will be reported.

GC stop the world will stop DeadlockWarningThread too, so it will not report false deadlock.


- Dmytro


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35074/#review87040
-----------------------------------------------------------


On June 8, 2015, 10:50 a.m., Vitalyi Brodetskyi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35074/
> -----------------------------------------------------------
> 
> (Updated June 8, 2015, 10:50 a.m.)
> 
> 
> Review request for Ambari, Jonathan Hurley and John Speidel.
> 
> 
> Bugs: AMBARI-11692
>     https://issues.apache.org/jira/browse/AMBARI-11692
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Fails on 2 ubuntu workstations
> 
> {noformat}
> 
> Tests in error:
>   testDeadlockWhileRestartingComponents(org.apache.ambari.server.state.cluster.ClusterDeadlockTest):
test timed out after 75000 milliseconds
> 
> Tests run: 3016, Failures: 0, Errors: 1, Skipped: 21
> 
> [INFO] ------------------------------------------------------------------------
> [INFO] Reactor Summary:
> [INFO]
> [INFO] Ambari Views ...................................... SUCCESS [4.863s]
> [INFO] Ambari Server ..................................... FAILURE [1:49:50.358s]
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD FAILURE
> 
> {noformat}
> 
> {noformat}
> java.lang.Exception: test timed out after 75000 milliseconds
> 	at java.lang.Object.wait(Native Method)
> 	at java.lang.Thread.join(Thread.java:1281)
> 	at java.lang.Thread.join(Thread.java:1355)
> 	at org.apache.ambari.server.state.cluster.ClusterDeadlockTest.testDeadlockWhileRestartingComponents(ClusterDeadlockTest.java:241)
> 	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> 	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
> 	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> 	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:45)
> 	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
> 	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:42)
> 	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20)
> 	at org.junit.internal.runners.statements.FailOnTimeout$StatementThread.run(FailOnTimeout.java:62)
> 
> 	at org.apache.ambari.server.state.svccomphost.ServiceComponentHostImpl.getDesiredStateEntity(ServiceComponentHostImpl.java:1555)
> 	at org.apache.ambari.server.state.svccomphost.ServiceComponentHostImpl.isRestartRequired(ServiceComponentHostImpl.java:1478)
> 	at org.apache.ambari.server.state.ConfigHelper.calculateIsStaleConfigs(ConfigHelper.java:927)
> 	at org.apache.ambari.server.state.ConfigHelper.isStaleConfigs(ConfigHelper.java:376)
> 	at org.apache.ambari.server.state.cluster.ClusterImpl.getClusterHealthReport(ClusterImpl.java:2580)
> {noformat}
> 
> Reproduced in trunk, last commit
> {noformat}
> commit a52eb51d1af0edc9273a947535a2a36886e625da
> Author: Oleg Nechiporenko <onechiporenko@apache.org>
> Date:   Thu May 28 18:02:28 2015 +0300
> 
>     AMBARI-11484. Configs: when doing override, it's hard to find config override (onechiporenko)
> 
> {noformat}
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/test/java/org/apache/ambari/server/state/cluster/ClusterDeadlockTest.java
2f064ab 
>   ambari-server/src/test/java/org/apache/ambari/server/state/cluster/DeadlockWarningThread.java
PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/state/cluster/DeadlockedThreadsTest.java
PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/35074/diff/
> 
> 
> Testing
> -------
> 
> mvn clean test
> 
> 
> Thanks,
> 
> Vitalyi Brodetskyi
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message