symat commented on a change in pull request #1356: URL: https://github.com/apache/zookeeper/pull/1356#discussion_r427212264
########## File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java ########## @@ -1508,20 +1510,25 @@ private synchronized void startZkServer() { newLeaderProposal.ackSetsToString(), Long.toHexString(zk.getZxid())); - /* - * ZOOKEEPER-1324. the leader sends the new config it must complete - * to others inside a NEWLEADER message (see LearnerHandler where - * the NEWLEADER message is constructed), and once it has enough - * acks we must execute the following code so that it applies the - * config to itself. - */ - QuorumVerifier newQV = self.getLastSeenQuorumVerifier(); + if (QuorumPeerConfig.isReconfigEnabled()) { + /* + * ZOOKEEPER-1324. the leader sends the new config it must complete + * to others inside a NEWLEADER message (see LearnerHandler where + * the NEWLEADER message is constructed), and once it has enough + * acks we must execute the following code so that it applies the + * config to itself. + */ + QuorumVerifier newQV = self.getLastSeenQuorumVerifier(); - Long designatedLeader = getDesignatedLeader(newLeaderProposal, zk.getZxid()); + Long designatedLeader = getDesignatedLeader(newLeaderProposal, zk.getZxid()); - self.processReconfig(newQV, designatedLeader, zk.getZxid(), true); - if (designatedLeader != self.getId()) { - allowedToCommit = false; + self.processReconfig(newQV, designatedLeader, zk.getZxid(), true); + if (designatedLeader != self.getId()) { + LOG.warn("This leader is not the designated leader, it will be initialized with allowedToCommit = false"); + allowedToCommit = false; + } + } else { + LOG.debug("Reconfig feature is disabled, skip designatedLeader calculation and reconfig processing."); Review comment: I agree with the LOG.info (this message will be printed only once when the leader starts to lead after an election. Yet, the message can help debugging production issues, when debug logs are disabled) The tests are simulating the "backward compatibility" case when `QuorumPeerConfig.isReconfigEnabled()==false`. I don't think that changing the config with rolling-restart would be advised when dynamic-reconfig is enabled. I think when the reconfig is enabled, the new config can propagate during the leader election, so the rolling-restart will be unneccessary. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org