You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2021/03/24 22:56:13 UTC

[GitHub] [zookeeper] functioner opened a new pull request #1653: ZOOKEEPER-4260: Backport ZOOKEEPER-3575 to branch-3.6

functioner opened a new pull request #1653:
URL: https://github.com/apache/zookeeper/pull/1653


   (cherry picked from commit 7c1251dbcf6a314466024f71ae5757bde34bb3fd)


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



[GitHub] [zookeeper] muse-dev[bot] commented on a change in pull request #1653: ZOOKEEPER-4260: Backport ZOOKEEPER-3575 to branch-3.6

Posted by GitBox <gi...@apache.org>.
muse-dev[bot] commented on a change in pull request #1653:
URL: https://github.com/apache/zookeeper/pull/1653#discussion_r600932669



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java
##########
@@ -152,13 +165,27 @@ void validateSession(ServerCnxn cnxn, long clientId, int timeout) throws IOExcep
     }
 
     /**
-     * write a packet to the leader
+     * write a packet to the leader.
+     *
+     * This method is called by multiple threads. We need to make sure that only one thread is writing to leaderOs at a time.
+     * When packets are sent synchronously, writing is done within a synchronization block.
+     * When packets are sent asynchronously, sender.queuePacket() is called, which writes to a BlockingQueue, which is thread-safe.
+     * Reading from this BlockingQueue and writing to leaderOs is the learner sender thread only.
+     * So we have only one thread writing to leaderOs at a time in either case.
      *
      * @param pp
      *                the proposal packet to be sent to the leader
      * @throws IOException
      */
     void writePacket(QuorumPacket pp, boolean flush) throws IOException {
+        if (asyncSending) {
+            sender.queuePacket(pp);
+        } else {
+            writePacketNow(pp, flush);
+        }
+    }
+
+    void writePacketNow(QuorumPacket pp, boolean flush) throws IOException {
         synchronized (leaderOs) {

Review comment:
       *THREAD_SAFETY_VIOLATION:*  Read/Write race. Non-private method `Learner.writePacketNow(...)` reads without synchronization from `this.leaderOs`. Potentially races with write in method `Learner.connectToLeader(...)`.
    Reporting because this access may occur on a background thread.
   (at-me [in a reply](https://docs.muse.dev/docs/talk-to-muse/) with `help` or `ignore`)




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



[GitHub] [zookeeper] functioner commented on pull request #1653: ZOOKEEPER-4260: Backport ZOOKEEPER-3575 to branch-3.6

Posted by GitBox <gi...@apache.org>.
functioner commented on pull request #1653:
URL: https://github.com/apache/zookeeper/pull/1653#issuecomment-809038276


   @arshadmohammad I cherry-pick #1645. Some configurations are not available in branch-3.6 so I do not include them.


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



[GitHub] [zookeeper] arshadmohammad commented on a change in pull request #1653: ZOOKEEPER-4260: Backport ZOOKEEPER-3575 to branch-3.6

Posted by GitBox <gi...@apache.org>.
arshadmohammad commented on a change in pull request #1653:
URL: https://github.com/apache/zookeeper/pull/1653#discussion_r603098693



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerHandler.java
##########
@@ -49,6 +49,7 @@
 import org.apache.zookeeper.server.quorum.Leader.Proposal;
 import org.apache.zookeeper.server.quorum.QuorumPeer.LearnerType;
 import org.apache.zookeeper.server.quorum.auth.QuorumAuthServer;
+import org.apache.zookeeper.server.util.ConfigUtils;

Review comment:
       This is unused import. remove it




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



[GitHub] [zookeeper] functioner commented on a change in pull request #1653: ZOOKEEPER-4260: Backport ZOOKEEPER-3575 to branch-3.6

Posted by GitBox <gi...@apache.org>.
functioner commented on a change in pull request #1653:
URL: https://github.com/apache/zookeeper/pull/1653#discussion_r600939750



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java
##########
@@ -152,13 +165,27 @@ void validateSession(ServerCnxn cnxn, long clientId, int timeout) throws IOExcep
     }
 
     /**
-     * write a packet to the leader
+     * write a packet to the leader.
+     *
+     * This method is called by multiple threads. We need to make sure that only one thread is writing to leaderOs at a time.
+     * When packets are sent synchronously, writing is done within a synchronization block.
+     * When packets are sent asynchronously, sender.queuePacket() is called, which writes to a BlockingQueue, which is thread-safe.
+     * Reading from this BlockingQueue and writing to leaderOs is the learner sender thread only.
+     * So we have only one thread writing to leaderOs at a time in either case.
      *
      * @param pp
      *                the proposal packet to be sent to the leader
      * @throws IOException
      */
     void writePacket(QuorumPacket pp, boolean flush) throws IOException {
+        if (asyncSending) {
+            sender.queuePacket(pp);
+        } else {
+            writePacketNow(pp, flush);
+        }
+    }
+
+    void writePacketNow(QuorumPacket pp, boolean flush) throws IOException {
         synchronized (leaderOs) {

Review comment:
       If `learner.asyncSending` is not set, all `QuorumPacket` will be sent in this `writePacketNow` method.
   If `learner.asyncSending` is set, all `QuorumPacket` will be sent in the `LearnerSender` thread, which doesn't need the synchronization.
   Therefore, there wouldn't be potential race.




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



[GitHub] [zookeeper] arshadmohammad closed pull request #1653: ZOOKEEPER-4260: Backport ZOOKEEPER-3575 to branch-3.6

Posted by GitBox <gi...@apache.org>.
arshadmohammad closed pull request #1653:
URL: https://github.com/apache/zookeeper/pull/1653


   


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



[GitHub] [zookeeper] arshadmohammad commented on pull request #1653: ZOOKEEPER-4260: Backport ZOOKEEPER-3575 to branch-3.6

Posted by GitBox <gi...@apache.org>.
arshadmohammad commented on pull request #1653:
URL: https://github.com/apache/zookeeper/pull/1653#issuecomment-809518628


   Merged


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



[GitHub] [zookeeper] arshadmohammad commented on pull request #1653: ZOOKEEPER-4260: Backport ZOOKEEPER-3575 to branch-3.6

Posted by GitBox <gi...@apache.org>.
arshadmohammad commented on pull request #1653:
URL: https://github.com/apache/zookeeper/pull/1653#issuecomment-809179189


   Other than minor import issues, changes Looks good to me. @functioner please correct the import


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



[GitHub] [zookeeper] arshadmohammad commented on pull request #1653: ZOOKEEPER-4260: Backport ZOOKEEPER-3575 to branch-3.6

Posted by GitBox <gi...@apache.org>.
arshadmohammad commented on pull request #1653:
URL: https://github.com/apache/zookeeper/pull/1653#issuecomment-808922810


   Please include the latest changes done in PR https://github.com/apache/zookeeper/pull/1645.
   Also add the documentation for learner.asyncSending property


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