[
https://issues.apache.org/jira/browse/ZOOKEEPER-3340?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
David Mollitor updated ZOOKEEPER-3340:
--
Description:
I was recently profiling a on a ZK Quorum Leader in a low-volume environment
and noticed that most of its time was spent in {{QuorumCnxManager#RecvWorker}}.
Nothing wrong with that, but it did draw my attention to it. I noticed that
{{Queue}} interactions are a bit... verbose. I would like to propose that we
streamline this area of the code.
[https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumCnxManager.java#L1291-L1309]
This proposed JIRA 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:
{code}
public void addToRecvQueue(Message msg) {
synchronized(recvQLock) {
if (recvQueue.remainingCapacity() == 0) {
try {
{code}
>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.
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."
was:
I was recently profiling a on a ZK Quorum Leader in a low-volume environment
and noticed that most of its time was spent in {{QuorumCnxManager#RecvWorker}}.
Nothing wrong with that, but it did draw my attention to it. I noticed that
{{Queue}} interactions are a bit... verbose. I would like to propose that we
streamline this area of the code.
[https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumCnxManager.java#L1291-L1309]
> Improve Queue Usage in QuorumCnxManager.java
>
>
> Key: ZOOKEEPER-3340
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-3340
> Project: ZooKeeper
> Issue Type: Improvement
> Components: server
>Reporter: David Mollitor
>Assignee: David Mollitor
>Priority: Major
> Labels: pull-request-available
> Fix For: 3.6.0
>
> Time Spent: 2h 20m
> Remaining Estimate: 0h
>
> I was recently profiling a on a ZK Quorum Leader in a low-volume environment
> and noticed that most of its time was spent in
> {{QuorumCnxManager#RecvWorker}}. Nothing wrong with that, but it did draw my
> attention to it. I noticed that {{Queue}} interactions are a bit... verbose.
> I would like to propose that we streamline this area of the code.
>
> [https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumCnxManager.java#L1291-L1309]
> This proposed JIRA 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:
> {code}
> public void addToRecvQueue(Message msg) {
> synchronized(recvQLock) {
> if (recvQueue.remainingCapacity() == 0) {
> try {
> {code}
> 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. 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
