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

    https://github.com/apache/zookeeper/pull/92#discussion_r97266698
  
    --- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java ---
    @@ -468,31 +469,33 @@ synchronized private boolean connectOne(long sid, 
InetSocketAddress electionAddr
          */
         
         synchronized void connectOne(long sid){
    +        connectOne(sid, self.getLastSeenQuorumVerifier());
    +    }
    +
    +    synchronized void connectOne(long sid, QuorumVerifier lastSeenQV){
             if (senderWorkerMap.get(sid) != null) {
    -             LOG.debug("There is a connection already for server " + sid);
    -             return;
    +            LOG.debug("There is a connection already for server " + sid);
    +            return;
             }
    -        synchronized(self) {
    -           boolean knownId = false;
    -            // Resolve hostname for the remote server before attempting to
    -            // connect in case the underlying ip address has changed.
    -            self.recreateSocketAddresses(sid);
    -            if (self.getView().containsKey(sid)) {
    -               knownId = true;
    -                if (connectOne(sid, self.getView().get(sid).electionAddr))
    -                   return;
    -            } 
    -            if (self.getLastSeenQuorumVerifier()!=null && 
self.getLastSeenQuorumVerifier().getAllMembers().containsKey(sid)
    -                   && (!knownId || 
(self.getLastSeenQuorumVerifier().getAllMembers().get(sid).electionAddr !=
    -                   self.getView().get(sid).electionAddr))) {
    -               knownId = true;
    -                if (connectOne(sid, 
self.getLastSeenQuorumVerifier().getAllMembers().get(sid).electionAddr))
    -                   return;
    -            } 
    -            if (!knownId) {
    -                LOG.warn("Invalid server id: " + sid);
    +        boolean knownId = false;
    +        // Resolve hostname for the remote server before attempting to
    +        // connect in case the underlying ip address has changed.
    +        self.recreateSocketAddresses(sid);
    +        if (self.getView().containsKey(sid)) {
    --- End diff --
    
    How about passing also the last committed view so you don't need to call 
getView() multiple times ?
    I know you're protecting this with the lock in QuorumPeer, but before I 
read the other file I thought there may be a race because of multiple accesses 
to the config. A comment would help here.
    



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to