ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Speidel" <jspei...@hortonworks.com>
Subject Re: Review Request 35074: ClusterDeadlockTest unit test fails
Date Mon, 08 Jun 2015 16:57:18 GMT

-----------------------------------------------------------
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 <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