[jira] [Commented] (ZOOKEEPER-2080) ReconfigRecoveryTest fails intermittently

["ASF GitHub Bot (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/ZOOKEEPER-2080?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15718771#comment-15718771 ] 

ASF GitHub Bot commented on ZOOKEEPER-2080:
-------------------------------------------

Github user lvfangmin commented on a diff in the pull request:

    https://github.com/apache/zookeeper/pull/92#discussion_r90764675
  
    --- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
    @@ -1390,24 +1406,29 @@ public QuorumVerifier configFromString(String s) throws IOException, ConfigExcep
         }
         
         /**
    -     * Return QuorumVerifier object for the last committed configuration
    +     * Return QuorumVerifier object for the last committed configuration.
          */
    -
    -    public synchronized QuorumVerifier getQuorumVerifier(){
    -        return quorumVerifier;
    -
    +    public QuorumVerifier getQuorumVerifier(){
    +        synchronized (qvLock) {
    +            return quorumVerifier;
    +        }
         }
     
    -    public synchronized QuorumVerifier getLastSeenQuorumVerifier(){
    -        return lastSeenQuorumVerifier;        
    +    /**
    +     * Return QuorumVerifier object for the last proposed configuration.
    +     */
    +    public QuorumVerifier getLastSeenQuorumVerifier(){
    +        synchronized (qvLock) {
    +            return lastSeenQuorumVerifier;
    +        }
         }
         
    -    public synchronized void connectNewPeers(){
    -       if (qcm!=null && getQuorumVerifier()!=null && getLastSeenQuorumVerifier()!=null) {
    -           Map<Long, QuorumServer> committedView = getQuorumVerifier().getAllMembers();
    -           for (Entry<Long, QuorumServer> e: getLastSeenQuorumVerifier().getAllMembers().entrySet()){
    +    private void connectNewPeers(){
    --- End diff --
    
    Hi @hanm, I've followed the long discussion on the Jira, thanks for digging deeper into the problem. LGTM, only a small suggestion: it's error-prone to assume that in the future this method won't be called by other non-synchronized methods, we'd better to add the synchronize(qvLock) here too.


> ReconfigRecoveryTest fails intermittently
> -----------------------------------------
>
>                 Key: ZOOKEEPER-2080
>                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2080
>             Project: ZooKeeper
>          Issue Type: Sub-task
>            Reporter: Ted Yu
>            Assignee: Michael Han
>             Fix For: 3.5.3, 3.6.0
>
>         Attachments: ZOOKEEPER-2080.patch, ZOOKEEPER-2080.patch, ZOOKEEPER-2080.patch, ZOOKEEPER-2080.patch, ZOOKEEPER-2080.patch, ZOOKEEPER-2080.patch, jacoco-ZOOKEEPER-2080.unzip-grows-to-70MB.7z, repro-20150816.log, threaddump.log
>
>
> I got the following test failure on MacBook with trunk code:
> {code}
> Testcase: testCurrentObserverIsParticipantInNewConfig took 93.628 sec
>   FAILED
> waiting for server 2 being up
> junit.framework.AssertionFailedError: waiting for server 2 being up
>   at org.apache.zookeeper.server.quorum.ReconfigRecoveryTest.testCurrentObserverIsParticipantInNewConfig(ReconfigRecoveryTest.java:529)
>   at org.apache.zookeeper.JUnit4ZKTestRunner$LoggedInvokeMethod.evaluate(JUnit4ZKTestRunner.java:52)
> {code}



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)