----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35074/#review87040 -----------------------------------------------------------
ambari-server/src/test/java/org/apache/ambari/server/state/cluster/ClusterDeadlockTest.java <https://reviews.apache.org/r/35074/#comment139298> why do you throw exceptions here? Why not just do assertions inline to be more clear? ambari-server/src/test/java/org/apache/ambari/server/state/cluster/ClusterDeadlockTest.java <https://reviews.apache.org/r/35074/#comment139299> same comment as line 214 ambari-server/src/test/java/org/apache/ambari/server/state/cluster/ClusterDeadlockTest.java <https://reviews.apache.org/r/35074/#comment139300> same comment as line 214 ambari-server/src/test/java/org/apache/ambari/server/state/cluster/DeadlockWarningThread.java <https://reviews.apache.org/r/35074/#comment139307> 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. ambari-server/src/test/java/org/apache/ambari/server/state/cluster/DeadlockWarningThread.java <https://reviews.apache.org/r/35074/#comment139302> why not findDeadlockedThreads()? ambari-server/src/test/java/org/apache/ambari/server/state/cluster/DeadlockWarningThread.java <https://reviews.apache.org/r/35074/#comment139303> why are there 2 different mechanisms for detecting deadlock here? mbean.findMonitorDeadlockedThreads() and then all of this code in the else block ambari-server/src/test/java/org/apache/ambari/server/state/cluster/DeadlockWarningThread.java <https://reviews.apache.org/r/35074/#comment139306> 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 ambari-server/src/test/java/org/apache/ambari/server/state/cluster/DeadlockWarningThread.java <https://reviews.apache.org/r/35074/#comment139305> 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? ambari-server/src/test/java/org/apache/ambari/server/state/cluster/DeadlockWarningThread.java <https://reviews.apache.org/r/35074/#comment139304> 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. - John Speidel 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 > >
