zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Mollitor (JIRA)" <j...@apache.org>
Subject [jira] [Updated] (ZOOKEEPER-3340) Improve Queue Usage in QuorumCnxManager.java
Date Mon, 22 Apr 2019 15:12:00 GMT

     [ 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 nuance of "why
is the code doing this."



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Mime
View raw message