BELUGABEHR commented on issue #880: ZOOKEEPER-3340: Improve Queue Usage in QuorumCnxManager.java URL: https://github.com/apache/zookeeper/pull/880#issuecomment-482817330 @lvfangmin Thank you for the review. So, this proposed PR should not be viewed simply as 'ArrayBlockingQueue' v.s. 'CircularBlockingQueue'. One of the things that this PR does is remove the need for double-locking. For example in ```addToRecvQueue``` the following condition exists: ``` public void addToRecvQueue(Message msg) { synchronized(recvQLock) { if (recvQueue.remainingCapacity() == 0) { try { ``` From here it can be observed that there are two locks obtained: ```recvQLock``` and the one internal to ```recvQueue```. This is required because there are multiple interactions that this Manager wants to do on the queue in a serialized way. The CircularBlockingQueue performs all of those actions on behalf of the caller, but it does it internal to the queue, under a single lock,... the one internal to CircularBlockingQueue. The current code also has some race-conditions that are simply ignored when they happen. The race conditions are detailed nicely in the code comments [here](https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumCnxManager.java#L1270-L1285). However, the changes in this PR directly deal with, and eliminate, these race conditions altogether since all actions that work against the CircularBlockingQueue all occur within its internal locks. This greatly simplifies the code and removes the need for new folks to learn this nuance of "why is the code doing this."
---------------------------------------------------------------- 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: [email protected] With regards, Apache Git Services
