I see that this has failed a few precommit builds now, and someone reported it failing in their local env regularly. Do we think this is just a general transient test, or was there a change checked in recently that might related to this code and its new transient failures? Perhaps it is just the Thread.sleep(1000) needs to be bumped up.

C

Such uses of sleep are just asking for trouble. Take a look at the use
of sleep in testSessionMove in the same class for a better way to do
this. I had gone through all the tests a while back, replacing all the
"sleep(x)" with something like this testSessionMove pattern (retry
with a max limit that's very long). During reviews we should look for
anti-patterns like this and address them before commit.

Patrick

On Tue, Jun 21, 2011 at 12:38 PM, Fournier, Camille F.
<[EMAIL PROTECTED]> wrote:
> I see that this has failed a few precommit builds now, and someone reported it failing in their local env regularly. Do we think this is just a general transient test, or was there a change checked in recently that might related to this code and its new transient failures? Perhaps it is just the Thread.sleep(1000) needs to be bumped up.
>
> C
>

On 6/21/11 12:45 PM, Patrick Hunt wrote:
> Such uses of sleep are just asking for trouble. Take a look at the use
> of sleep in testSessionMove in the same class for a better way to do
> this. I had gone through all the tests a while back, replacing all the
> "sleep(x)" with something like this testSessionMove pattern (retry
> with a max limit that's very long). During reviews we should look for
> anti-patterns like this and address them before commit.
>
> Patrick
Thanks a lot for bringing this up, Camille. I had exactly this problem
(QuorumTest.testFollowersStartAfterLeader failing) yesterday and today .
Would the attached patch be the fix in the spirit of the pattern you're
describing, Patrick?

-Eugene

Hi Eugene, that looks right to me. (did that fix it for you?)

In addition to the anti-pattern I mentioned earlier, another one to
look for is slow running tests -- often times a test will run slowly
that could be coded in a different way to run much more quickly.
Notice here that we don't even wait the second if the test is already
passing. Although we might run for a much longer time if needed. (this
all adds up when you have hundreds/thousands of tests).

btw, you might put that comment directly into your assert. Also take a
look at Assert.fail("foo") instead of assertTrue(false). (it's a nit
though).

Patrick

On Tue, Jun 21, 2011 at 2:03 PM, Eugene Koontz <[EMAIL PROTECTED]> wrote:
> On 6/21/11 12:45 PM, Patrick Hunt wrote:
>>
>> Such uses of sleep are just asking for trouble. Take a look at the use
>> of sleep in testSessionMove in the same class for a better way to do
>> this. I had gone through all the tests a while back, replacing all the
>> "sleep(x)" with something like this testSessionMove pattern (retry
>> with a max limit that's very long). During reviews we should look for
>> anti-patterns like this and address them before commit.
>>
>> Patrick
>
> Thanks a lot for bringing this up, Camille. I had exactly this problem
> (QuorumTest.testFollowersStartAfterLeader failing) yesterday and today .
> Would the attached patch be the fix in the spirit of the pattern you're
> describing, Patrick?
>
> -Eugene
>
>
>

On 6/21/11 2:12 PM, Patrick Hunt wrote:
> Hi Eugene, that looks right to me. (did that fix it for you?)
>
> In addition to the anti-pattern I mentioned earlier, another one to
> look for is slow running tests -- often times a test will run slowly
> that could be coded in a different way to run much more quickly.
> Notice here that we don't even wait the second if the test is already
> passing. Although we might run for a much longer time if needed. (this
> all adds up when you have hundreds/thousands of tests).
>
> btw, you might put that comment directly into your assert. Also take a
> look at Assert.fail("foo") instead of assertTrue(false). (it's a nit
> though).
>
> Patrick
>
Hi Patrick,

I created https://issues.apache.org/jira/browse/ZOOKEEPER-1103 and added
a patch. I used Assert.fail() as you recommended. Your comments
regarding avoiding unnecessary sleep()s are well said - I hate waiting
for long tests! :)

-Eugene

On 6/21/11 2:33 PM, Eugene Koontz wrote:
> On 6/21/11 2:12 PM, Patrick Hunt wrote:
>> Hi Eugene, that looks right to me. (did that fix it for you?)
>>
Forgot to say; yes, it does fix QuorumTest for me. I used the attached
script file to run this test repeatedly until failure, and it never exited.
-Eugene

Cool, thanks for the patch. Looks like this effects both 3.3 and trunk.

On Tue, Jun 21, 2011 at 2:37 PM, Eugene Koontz <[EMAIL PROTECTED]> wrote:
> On 6/21/11 2:33 PM, Eugene Koontz wrote:
>>
>> On 6/21/11 2:12 PM, Patrick Hunt wrote:
>>>
>>> Hi Eugene, that looks right to me. (did that fix it for you?)
>>>
> Forgot to say; yes, it does fix QuorumTest for me. I used the attached
> script file to run this test repeatedly until failure, and it never exited.
> -Eugene
>