zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From hanm <...@git.apache.org>
Subject [GitHub] zookeeper issue #229: ZOOKEEPER-2759: Flaky test: org.apache.zookeeper.serve...
Date Wed, 26 Apr 2017 05:43:08 GMT
Github user hanm commented on the issue:

    https://github.com/apache/zookeeper/pull/229
  
    Thanks for work on this! I hope we have more contributors focus on improving the test.
    
    The proposed fix masks the real problem - it will always success no matter what - even
if the connection was succeeded, since the halt will clean up the sender worker map of the
peer 0.
    
    >>  a race condition between the removal of the relevant sid from senderWorkerMap
for peer0 and the 3 second delay from assertEventuallyNotConnected.
    I think there is a race, but my observation on it is slightly different. The race is between
when peer 0 received (and finished) establish connection with peer 1, and the assert inside
assertEventuallyNotConnected.
    
    >> may not destroy them in time for the test assertion.
    Because of the race, once peer 0 has received the connection, its sender worker map will
has sid 1 entry forever until the death of the peer 0 itself. 
    
    I think instead of checking on peer 0, we can check peer 1. Its sender worker map should
always be clean. We can't check peer 0 here because the current API does not encode the true
state of the connection - it only encodes what happened (the connection from 1 to 0 was established
on 0's side), but it does not encode the status of the connection (is the connection still
alive or not - in this case peer 1 will close the socket later when it found out peer 0 has
no auth. but on peer 0 side the sender work will have sid 1 inside until its death, what's
done has been done.). The state of peer 1 wrt its sender map should also reflect state of
peer 0  since there is only a single connection between 0 and 1.
    
    Something like this should serve the purpose:
    assertEventuallyNotConnected(peer1, 0);
    
    Maybe add a sleep before it to make sure all the connection attempts were made (from 0
 to 1 and from 1 to 0 two ways), but it's a best effort. 5 sec sounds enough.
    



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message