> 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 <[email protected]> > 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 > >
