ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jun aoki" <jun.aoki....@gmail.com>
Subject Re: Review Request 26510: AMBARI-7622 TestActionScheduler fails occasionally on builds.a.o stating expected:<ABORTED> but was:<PENDING>
Date Mon, 13 Oct 2014 18:30:55 GMT


> On Oct. 10, 2014, 6:49 p.m., John Speidel wrote:
> > I don't feel comfortable with this test being merged as is.
> > If you need to resolve this before a proper fix is implemented, please ignore the
test via @Ignore and file a Jira to properly fix the test.
> 
> jun aoki wrote:
>     Hi John, I hear your concern and agree with you two that the test should not use
asynchronous. 
>     But the test has been this way since 2012 and I am hesitent to drop the test coverage
by adding @Ignore.
>     https://github.com/apache/ambari/commits/trunk/ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionScheduler.java
> 
> John Speidel wrote:
>     Jun, first off thanks for taking the time to analyze this intermittent test failure.
>     I spent the last hour looking at this test.  
>     My conclusion is that this is a very poorly written test from it's orgin.  It is
way too coarse grained, uses the NPE as an intentional error mechanism and expects several
iterations of doWork() to complete based on the states returned by the test mocks.
>     Your changes, although they result in a passing test now, have several issues.
>     - They circumvent the run method which under the case being tested, catches the NPE
and makes some state changes
>     - The changes are not written in a way that guarantees a deterministic behavior:
"while(count < 10)" is a big red flag
>     - The changes will make the test VERY confusing to anybody who needs to fix these
tests in the future
>     
>     All of the tests in this class need to be rewrittn in a way that is deterministic
and doesn't require obscure NPE exceptions or multiple interations of doWork().
>     
>     I still feel it is better to @Ignore the test, or all of them in this class for that
matter, and have them fixed properly when possible.
> 
> jun aoki wrote:
>     Hi John, thank you for writing down to share details of you thoughts. I agree all
3 bullets you mentioned, but only in my second patch.
>     
>     My first intention and patch is much simpler and to fix a "wrong if statement".
>     https://reviews.apache.org/r/26510/diff/1/
>     
>     I still hesitant to drop the test coverage, but let me know if I don't still convince
you, I will just set a @Ignore on the test.
> 
> John Speidel wrote:
>     Jun,
>     
>     The change that you made in patch 1 fixes a race condition where we could break out
of the while loop if only one of the 2 conditiona are satisfied.
>     But, I still have a serious concern that this test could/will at some point loop
forever because one or both of the conditions are not satisfied.
>     
>     So, I will relunctantly agree to +1 the patch if 
>     - you add a relief valve to the test so that it fails out if the conditions aren't
met in some period of time (something like 5 mins or we risk lots of intermittent failures)
>     - a Jira is filed to properly fix these tests in the near future
>     - we agree that the next time the test fails that we @Ignore it

John, thank you for spending a good amount of time to do the right thing. I see your concerns
valid too. 
I did not know ActionScheduler has a chance not to change the state forever. It would be really
bad because then it takes up a Jenkins slave until killed manually. 
I will just mark it as @Ignored and let Sid fix with the right approach.  Will attach a new
one shortly.
John/Sid, thank you again for your quite a lot of time fot this!


- jun


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


On Oct. 9, 2014, 10:05 p.m., jun aoki wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26510/
> -----------------------------------------------------------
> 
> (Updated Oct. 9, 2014, 10:05 p.m.)
> 
> 
> Review request for Ambari and Yusaku Sako.
> 
> 
> Bugs: AMBARI-7622
>     https://issues.apache.org/jira/browse/AMBARI-7622
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Tweaked the waiting condition upon ActionScheduler
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionScheduler.java
a20f252 
> 
> Diff: https://reviews.apache.org/r/26510/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> jun aoki
> 
>


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