zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mkedwards <...@git.apache.org>
Subject [GitHub] zookeeper pull request #707: [ZOOKEEPER-2778] QuorumPeer: encapsulate addres...
Date Wed, 21 Nov 2018 17:02:08 GMT
Github user mkedwards commented on a diff in the pull request:

    https://github.com/apache/zookeeper/pull/707#discussion_r235470421
  
    --- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeer.java
---
    @@ -1566,32 +1585,50 @@ public String getNextDynamicConfigFilename() {
         }
         
         public void setLastSeenQuorumVerifier(QuorumVerifier qv, boolean writeToDisk){
    -        synchronized (QV_LOCK) {
    -            if (lastSeenQuorumVerifier != null && lastSeenQuorumVerifier.getVersion()
> qv.getVersion()) {
    -                LOG.error("setLastSeenQuorumVerifier called with stale config " + qv.getVersion()
+
    -                        ". Current version: " + quorumVerifier.getVersion());
    +        // If qcm is non-null, we may call qcm.connectOne(), which will take the lock
on qcm
    +        // and then take QV_LOCK.  Take the locks in the same order to ensure that we
don't
    +        // deadlock against other callers of connectOne().  If qcmRef gets set in another
    +        // thread while we're inside the synchronized block, that does no harm; if we
didn't
    +        // take a lock on qcm (because it was null when we sampled it), we won't call
    +        // connectOne() on it.  (Use of an AtomicReference is enough to guarantee visibility
    +        // of updates that provably happen in another thread before entering this method.)
    +        QuorumCnxManager qcm = qcmRef.get();
    +        Object outerLockObject = (qcm != null) ? qcm : QV_LOCK;
    +        synchronized (outerLockObject) {
    +            synchronized (QV_LOCK) {
    +                if (lastSeenQuorumVerifier != null && lastSeenQuorumVerifier.getVersion()
> qv.getVersion()) {
    +                    LOG.error("setLastSeenQuorumVerifier called with stale config " +
qv.getVersion() +
    +                            ". Current version: " + quorumVerifier.getVersion());
    +                }
    +                // assuming that a version uniquely identifies a configuration, so if
    +                // version is the same, nothing to do here.
    +                if (lastSeenQuorumVerifier != null &&
    +                        lastSeenQuorumVerifier.getVersion() == qv.getVersion()) {
    +                    return;
    +                }
    +                lastSeenQuorumVerifier = qv;
     
    -            }
    -            // assuming that a version uniquely identifies a configuration, so if
    -            // version is the same, nothing to do here.
    -            if (lastSeenQuorumVerifier != null &&
    -                    lastSeenQuorumVerifier.getVersion() == qv.getVersion()) {
    -                return;
    -            }
    -            lastSeenQuorumVerifier = qv;
    -            connectNewPeers();
    --- End diff --
    
    I had folded it back in so I could see the whole flow with the nested calls to `qcm.connectOne()`.
 I'll factor it back out with `qcm` as a method parameter (it's important that the `AtomicReference`
`qcmRef` not be sampled again inside the `synchronized` block, since we could race against
another thread changing the election algorithm).


---

Mime
View raw message