[GitHub] [zookeeper] ztzg 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>]

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



##########
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) {
-        this.state = state;
+        synchronized (stateChangeMutex) {
+            if (this.state == State.ERROR) {
+                if (state == State.RUNNING || state == State.INITIAL) {
+                    return;

Review comment:
       100% agree with @eolivelli on adding a `LOG.warn` (if not `LOG.error`).
   
   For the test, I would suggest mocking and/or overriding some methods to explicitly generate the fault, if possible.  I haven't tried doing so, but perhaps `LearnerTest.java` (which subclasses `LearnerZooKeeperServer`) can serve as inspiration?

##########
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:
       I agree with @eolivelli that `setState()` has to be "careful," because `ZooKeeperServerListenerImpl.notifyStopping` calls it from "random" threads.
   
   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`.)




----------------------------------------------------------------
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