[GitHub] [zookeeper] functioner commented on a change in pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

[GitBox <gi...@apache.org>]

functioner commented on a change in pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#discussion_r588902426



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumZooKeeperServer.java
##########
@@ -189,9 +189,18 @@ public void dumpConf(PrintWriter pwriter) {
         pwriter.print(self.getQuorumVerifier().toString());
     }
 
+    private final Object stateChangeMutex = new Object();
+
     @Override
     protected void setState(State state) {

Review comment:
       > But @Tison's point still holds; it seems to me that this could be simplified by making `setState()` a `synchronized` method. Do you have a specific reason of using a separate object?
   > 
   > (The parent `setState()` does not _need_ to be `synchronized` because the field is `volatile`.)
   
   @ztzg The rationale for not making `setState()` a synchronized method is that if a thread is running another synchronized method of this ZooKeeperServer object for some time, it may block the `setState()` invoked by another thread, and this invocation may be critical.




----------------------------------------------------------------
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:
users@infra.apache.org