You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Skye Wanderman-Milne <sk...@cloudera.com> on 2012/10/25 02:50:03 UTC
Review Request: patch for ZOOKEEPER-1560: Zookeeper client hangs on creation
of large nodes
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7730/
-----------------------------------------------------------
Review request for zookeeper, Patrick Hunt and Ted Yu.
Description
-------
see ZOOKEEPER-1560 JIRA
This addresses bug ZOOKEEPER-1560.
https://issues.apache.org/jira/browse/ZOOKEEPER-1560
Diffs
-----
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 70d8538
Diff: https://reviews.apache.org/r/7730/diff/
Testing
-------
unit tests (including testLargeNodeData from ZOOKEEPER-1560 JIRA)
Thanks,
Skye Wanderman-Milne
Re: Review Request: patch for ZOOKEEPER-1560: Zookeeper client hangs on
creation of large nodes
Posted by Skye Wanderman-Milne <sk...@cloudera.com>.
> On Oct. 25, 2012, 9:23 p.m., Patrick Hunt wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, lines 133-134
> > <https://reviews.apache.org/r/7730/diff/1/?file=179601#file179601line133>
> >
> > why are we synchronizing this? it's already protected by outgoingQueue, is this not enough? (it was only protected by outgoingQueue in 3.3 branch, why did it change?)
pendingQueue is not always protected by outgoingQueue (see ClientCnxn.SendThread.readResponse, SendThread.cleanup). It's likely that these functions are never called concurrently with doIO but I'll have to look into it, and maybe we should document somewhere that pendingQueue should be protected by outgoingQueue (since right now it's not). I'll also look into what's going on in 3.3.
> On Oct. 25, 2012, 9:23 p.m., Patrick Hunt wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, lines 138-145
> > <https://reviews.apache.org/r/7730/diff/1/?file=179601#file179601line138>
> >
> > We should check if (outgoingQueue.isEmpty()) here and disableWrite if true. No need to wake up again if the queue is already empty.
> >
> > Honestly I'd just put it back to what it was in 3.3 -
> >
> > if (outgoingQueue.isEmpty()) {
> > disableWrite();
> > } else {
> > enableWrite();
> > }
> >
done
I don't think it's necessary to enableWrite because this is the only place disableWrite is called, but I did it anyway.
- Skye
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7730/#review12792
-----------------------------------------------------------
On Oct. 25, 2012, 10:51 p.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7730/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2012, 10:51 p.m.)
>
>
> Review request for zookeeper, Patrick Hunt and Ted Yu.
>
>
> Description
> -------
>
> see ZOOKEEPER-1560 JIRA
>
>
> This addresses bug ZOOKEEPER-1560.
> https://issues.apache.org/jira/browse/ZOOKEEPER-1560
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 70d8538
>
> Diff: https://reviews.apache.org/r/7730/diff/
>
>
> Testing
> -------
>
> unit tests (including testLargeNodeData from ZOOKEEPER-1560 JIRA)
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>
Re: Review Request: patch for ZOOKEEPER-1560: Zookeeper client hangs on
creation of large nodes
Posted by Patrick Hunt <ph...@apache.org>.
> On Oct. 25, 2012, 9:23 p.m., Patrick Hunt wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, lines 133-134
> > <https://reviews.apache.org/r/7730/diff/1/?file=179601#file179601line133>
> >
> > why are we synchronizing this? it's already protected by outgoingQueue, is this not enough? (it was only protected by outgoingQueue in 3.3 branch, why did it change?)
>
> Skye Wanderman-Milne wrote:
> pendingQueue is not always protected by outgoingQueue (see ClientCnxn.SendThread.readResponse, SendThread.cleanup). It's likely that these functions are never called concurrently with doIO but I'll have to look into it, and maybe we should document somewhere that pendingQueue should be protected by outgoingQueue (since right now it's not). I'll also look into what's going on in 3.3.
>
> Skye Wanderman-Milne wrote:
> In 3.3 the other references to pendingQueue (readResponse, cleanup) are synced on itself (and not outgoingQueue), so it looks like not syncing in doIO was an oversight and we should keep the synchronized block.
Ok. In that case can you verify that there are no lock ordering issues that might cause a deadlock?
- Patrick
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7730/#review12792
-----------------------------------------------------------
On Oct. 25, 2012, 10:51 p.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7730/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2012, 10:51 p.m.)
>
>
> Review request for zookeeper, Patrick Hunt and Ted Yu.
>
>
> Description
> -------
>
> see ZOOKEEPER-1560 JIRA
>
>
> This addresses bug ZOOKEEPER-1560.
> https://issues.apache.org/jira/browse/ZOOKEEPER-1560
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 70d8538
>
> Diff: https://reviews.apache.org/r/7730/diff/
>
>
> Testing
> -------
>
> unit tests (including testLargeNodeData from ZOOKEEPER-1560 JIRA)
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>
Re: Review Request: patch for ZOOKEEPER-1560: Zookeeper client hangs on
creation of large nodes
Posted by Skye Wanderman-Milne <sk...@cloudera.com>.
> On Oct. 25, 2012, 9:23 p.m., Patrick Hunt wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, lines 133-134
> > <https://reviews.apache.org/r/7730/diff/1/?file=179601#file179601line133>
> >
> > why are we synchronizing this? it's already protected by outgoingQueue, is this not enough? (it was only protected by outgoingQueue in 3.3 branch, why did it change?)
>
> Skye Wanderman-Milne wrote:
> pendingQueue is not always protected by outgoingQueue (see ClientCnxn.SendThread.readResponse, SendThread.cleanup). It's likely that these functions are never called concurrently with doIO but I'll have to look into it, and maybe we should document somewhere that pendingQueue should be protected by outgoingQueue (since right now it's not). I'll also look into what's going on in 3.3.
In 3.3 the other references to pendingQueue (readResponse, cleanup) are synced on itself (and not outgoingQueue), so it looks like not syncing in doIO was an oversight and we should keep the synchronized block.
- Skye
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7730/#review12792
-----------------------------------------------------------
On Oct. 25, 2012, 10:51 p.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7730/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2012, 10:51 p.m.)
>
>
> Review request for zookeeper, Patrick Hunt and Ted Yu.
>
>
> Description
> -------
>
> see ZOOKEEPER-1560 JIRA
>
>
> This addresses bug ZOOKEEPER-1560.
> https://issues.apache.org/jira/browse/ZOOKEEPER-1560
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 70d8538
>
> Diff: https://reviews.apache.org/r/7730/diff/
>
>
> Testing
> -------
>
> unit tests (including testLargeNodeData from ZOOKEEPER-1560 JIRA)
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>
Re: Review Request: patch for ZOOKEEPER-1560: Zookeeper client hangs on
creation of large nodes
Posted by Patrick Hunt <ph...@apache.org>.
> On Oct. 25, 2012, 9:23 p.m., Patrick Hunt wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, lines 133-134
> > <https://reviews.apache.org/r/7730/diff/1/?file=179601#file179601line133>
> >
> > why are we synchronizing this? it's already protected by outgoingQueue, is this not enough? (it was only protected by outgoingQueue in 3.3 branch, why did it change?)
>
> Skye Wanderman-Milne wrote:
> pendingQueue is not always protected by outgoingQueue (see ClientCnxn.SendThread.readResponse, SendThread.cleanup). It's likely that these functions are never called concurrently with doIO but I'll have to look into it, and maybe we should document somewhere that pendingQueue should be protected by outgoingQueue (since right now it's not). I'll also look into what's going on in 3.3.
>
> Skye Wanderman-Milne wrote:
> In 3.3 the other references to pendingQueue (readResponse, cleanup) are synced on itself (and not outgoingQueue), so it looks like not syncing in doIO was an oversight and we should keep the synchronized block.
>
> Patrick Hunt wrote:
> Ok. In that case can you verify that there are no lock ordering issues that might cause a deadlock?
>
> Skye Wanderman-Milne wrote:
> I don't see any potential deadlocks -- this is actually the only instance where both locks are held at the same time.
>
> It also appears pendingQueue is only used within the sendThread (except for ClientCnxn.toString), so I'm tempted to remove all synchronization on it completely.
Ok, that's what I wanted to verify.
- Patrick
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7730/#review12792
-----------------------------------------------------------
On Oct. 25, 2012, 10:51 p.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7730/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2012, 10:51 p.m.)
>
>
> Review request for zookeeper, Patrick Hunt and Ted Yu.
>
>
> Description
> -------
>
> see ZOOKEEPER-1560 JIRA
>
>
> This addresses bug ZOOKEEPER-1560.
> https://issues.apache.org/jira/browse/ZOOKEEPER-1560
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 70d8538
>
> Diff: https://reviews.apache.org/r/7730/diff/
>
>
> Testing
> -------
>
> unit tests (including testLargeNodeData from ZOOKEEPER-1560 JIRA)
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>
Re: Review Request: patch for ZOOKEEPER-1560: Zookeeper client hangs on
creation of large nodes
Posted by Skye Wanderman-Milne <sk...@cloudera.com>.
> On Oct. 25, 2012, 9:23 p.m., Patrick Hunt wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, lines 133-134
> > <https://reviews.apache.org/r/7730/diff/1/?file=179601#file179601line133>
> >
> > why are we synchronizing this? it's already protected by outgoingQueue, is this not enough? (it was only protected by outgoingQueue in 3.3 branch, why did it change?)
>
> Skye Wanderman-Milne wrote:
> pendingQueue is not always protected by outgoingQueue (see ClientCnxn.SendThread.readResponse, SendThread.cleanup). It's likely that these functions are never called concurrently with doIO but I'll have to look into it, and maybe we should document somewhere that pendingQueue should be protected by outgoingQueue (since right now it's not). I'll also look into what's going on in 3.3.
>
> Skye Wanderman-Milne wrote:
> In 3.3 the other references to pendingQueue (readResponse, cleanup) are synced on itself (and not outgoingQueue), so it looks like not syncing in doIO was an oversight and we should keep the synchronized block.
>
> Patrick Hunt wrote:
> Ok. In that case can you verify that there are no lock ordering issues that might cause a deadlock?
I don't see any potential deadlocks -- this is actually the only instance where both locks are held at the same time.
It also appears pendingQueue is only used within the sendThread (except for ClientCnxn.toString), so I'm tempted to remove all synchronization on it completely.
- Skye
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7730/#review12792
-----------------------------------------------------------
On Oct. 25, 2012, 10:51 p.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7730/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2012, 10:51 p.m.)
>
>
> Review request for zookeeper, Patrick Hunt and Ted Yu.
>
>
> Description
> -------
>
> see ZOOKEEPER-1560 JIRA
>
>
> This addresses bug ZOOKEEPER-1560.
> https://issues.apache.org/jira/browse/ZOOKEEPER-1560
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 70d8538
>
> Diff: https://reviews.apache.org/r/7730/diff/
>
>
> Testing
> -------
>
> unit tests (including testLargeNodeData from ZOOKEEPER-1560 JIRA)
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>
Re: Review Request: patch for ZOOKEEPER-1560: Zookeeper client hangs on
creation of large nodes
Posted by Patrick Hunt <ph...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7730/#review12792
-----------------------------------------------------------
There's a big problem here: if the packet went unsent (fully) then the next time around we try to pick up where we left off. That's broken though because we assume it's the first packet, and from what I can tell findSendable doesn't always return the first packet, and no one seems to be updating the queue.
We really should have a test case for this, not sure if it's possible in the current infrastructure though.
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java
<https://reviews.apache.org/r/7730/#comment27375>
why are we synchronizing this? it's already protected by outgoingQueue, is this not enough? (it was only protected by outgoingQueue in 3.3 branch, why did it change?)
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java
<https://reviews.apache.org/r/7730/#comment27374>
We should check if (outgoingQueue.isEmpty()) here and disableWrite if true. No need to wake up again if the queue is already empty.
Honestly I'd just put it back to what it was in 3.3 -
if (outgoingQueue.isEmpty()) {
disableWrite();
} else {
enableWrite();
}
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java
<https://reviews.apache.org/r/7730/#comment27377>
This isn't going to work - what if the packet was not originally at the head of the queue? I don't see where it's moved to the front.
- Patrick Hunt
On Oct. 25, 2012, 12:50 a.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7730/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2012, 12:50 a.m.)
>
>
> Review request for zookeeper, Patrick Hunt and Ted Yu.
>
>
> Description
> -------
>
> see ZOOKEEPER-1560 JIRA
>
>
> This addresses bug ZOOKEEPER-1560.
> https://issues.apache.org/jira/browse/ZOOKEEPER-1560
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 70d8538
>
> Diff: https://reviews.apache.org/r/7730/diff/
>
>
> Testing
> -------
>
> unit tests (including testLargeNodeData from ZOOKEEPER-1560 JIRA)
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>
Re: Review Request: patch for ZOOKEEPER-1560: Zookeeper client hangs on
creation of large nodes
Posted by Ted Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7730/#review12751
-----------------------------------------------------------
Ship it!
I think this patch nicely summarizes collective feedback for this JIRA.
Minor comments below.
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java
<https://reviews.apache.org/r/7730/#comment27226>
'p.bb will already exist' -> 'p.bb would not be null'
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java
<https://reviews.apache.org/r/7730/#comment27227>
'we already starting' -> 'we have already started'
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java
<https://reviews.apache.org/r/7730/#comment27228>
Remove white space introduced.
- Ted Yu
On Oct. 25, 2012, 12:50 a.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7730/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2012, 12:50 a.m.)
>
>
> Review request for zookeeper, Patrick Hunt and Ted Yu.
>
>
> Description
> -------
>
> see ZOOKEEPER-1560 JIRA
>
>
> This addresses bug ZOOKEEPER-1560.
> https://issues.apache.org/jira/browse/ZOOKEEPER-1560
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 70d8538
>
> Diff: https://reviews.apache.org/r/7730/diff/
>
>
> Testing
> -------
>
> unit tests (including testLargeNodeData from ZOOKEEPER-1560 JIRA)
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>
Re: Review Request: patch for ZOOKEEPER-1560: Zookeeper client hangs on
creation of large nodes
Posted by Nikita Vetoshkin <ni...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7730/#review12760
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java
<https://reviews.apache.org/r/7730/#comment27286>
Can't we move this check under p.bb == null?
- Nikita Vetoshkin
On Oct. 25, 2012, 12:50 a.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7730/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2012, 12:50 a.m.)
>
>
> Review request for zookeeper, Patrick Hunt and Ted Yu.
>
>
> Description
> -------
>
> see ZOOKEEPER-1560 JIRA
>
>
> This addresses bug ZOOKEEPER-1560.
> https://issues.apache.org/jira/browse/ZOOKEEPER-1560
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 70d8538
>
> Diff: https://reviews.apache.org/r/7730/diff/
>
>
> Testing
> -------
>
> unit tests (including testLargeNodeData from ZOOKEEPER-1560 JIRA)
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>
Re: Review Request: patch for ZOOKEEPER-1560: Zookeeper client hangs on
creation of large nodes
Posted by Skye Wanderman-Milne <sk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7730/#review12743
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java
<https://reviews.apache.org/r/7730/#comment27199>
Don't setXid > 1x for the same packet
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java
<https://reviews.apache.org/r/7730/#comment27200>
Don't createBB > 1x for the same packet
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java
<https://reviews.apache.org/r/7730/#comment27203>
Remove p from outgoingQueue only after we have finished writing it
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java
<https://reviews.apache.org/r/7730/#comment27205>
Always pick first packet if we already started writing it so we finished writing it
- Skye Wanderman-Milne
On Oct. 25, 2012, 12:50 a.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7730/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2012, 12:50 a.m.)
>
>
> Review request for zookeeper, Patrick Hunt and Ted Yu.
>
>
> Description
> -------
>
> see ZOOKEEPER-1560 JIRA
>
>
> This addresses bug ZOOKEEPER-1560.
> https://issues.apache.org/jira/browse/ZOOKEEPER-1560
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 70d8538
>
> Diff: https://reviews.apache.org/r/7730/diff/
>
>
> Testing
> -------
>
> unit tests (including testLargeNodeData from ZOOKEEPER-1560 JIRA)
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>
Re: Review Request: patch for ZOOKEEPER-1560: Zookeeper client hangs on
creation of large nodes
Posted by Skye Wanderman-Milne <sk...@cloudera.com>.
> On Oct. 25, 2012, 9:36 p.m., Patrick Hunt wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, lines 160-161
> > <https://reviews.apache.org/r/7730/diff/1/?file=179601#file179601line160>
> >
> > hm, maybe this is ok given the clientTunneledAuthenticationInProgress check. However it seems like it would be more straightforward to just reorder the queue, no?
It is OK (at least until something breaks it), but I've changed it to move the chosen auth packet to the front of the queue anyway.
- Skye
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7730/#review12794
-----------------------------------------------------------
On Oct. 25, 2012, 10:51 p.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7730/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2012, 10:51 p.m.)
>
>
> Review request for zookeeper, Patrick Hunt and Ted Yu.
>
>
> Description
> -------
>
> see ZOOKEEPER-1560 JIRA
>
>
> This addresses bug ZOOKEEPER-1560.
> https://issues.apache.org/jira/browse/ZOOKEEPER-1560
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 70d8538
>
> Diff: https://reviews.apache.org/r/7730/diff/
>
>
> Testing
> -------
>
> unit tests (including testLargeNodeData from ZOOKEEPER-1560 JIRA)
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>
Re: Review Request: patch for ZOOKEEPER-1560: Zookeeper client hangs on
creation of large nodes
Posted by Patrick Hunt <ph...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7730/#review12794
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java
<https://reviews.apache.org/r/7730/#comment27383>
hm, maybe this is ok given the clientTunneledAuthenticationInProgress check. However it seems like it would be more straightforward to just reorder the queue, no?
- Patrick Hunt
On Oct. 25, 2012, 12:50 a.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7730/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2012, 12:50 a.m.)
>
>
> Review request for zookeeper, Patrick Hunt and Ted Yu.
>
>
> Description
> -------
>
> see ZOOKEEPER-1560 JIRA
>
>
> This addresses bug ZOOKEEPER-1560.
> https://issues.apache.org/jira/browse/ZOOKEEPER-1560
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 70d8538
>
> Diff: https://reviews.apache.org/r/7730/diff/
>
>
> Testing
> -------
>
> unit tests (including testLargeNodeData from ZOOKEEPER-1560 JIRA)
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>
Re: Review Request: patch for ZOOKEEPER-1560: Zookeeper client hangs on
creation of large nodes
Posted by Patrick Hunt <ph...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7730/#review12938
-----------------------------------------------------------
Ship it!
lgtm. Thanks Skye, great job driving to completion.
- Patrick Hunt
On Oct. 25, 2012, 10:51 p.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7730/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2012, 10:51 p.m.)
>
>
> Review request for zookeeper, Patrick Hunt and Ted Yu.
>
>
> Description
> -------
>
> see ZOOKEEPER-1560 JIRA
>
>
> This addresses bug ZOOKEEPER-1560.
> https://issues.apache.org/jira/browse/ZOOKEEPER-1560
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 70d8538
>
> Diff: https://reviews.apache.org/r/7730/diff/
>
>
> Testing
> -------
>
> unit tests (including testLargeNodeData from ZOOKEEPER-1560 JIRA)
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>
Re: Review Request: patch for ZOOKEEPER-1560: Zookeeper client hangs on
creation of large nodes
Posted by Skye Wanderman-Milne <sk...@cloudera.com>.
> On Oct. 25, 2012, 11:33 p.m., Skye Wanderman-Milne wrote:
> > Looks like I'm no longer passing unit tests, please hold off on further reviews for now...
Just kidding, I was testing the wrong branch :)
- Skye
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7730/#review12800
-----------------------------------------------------------
On Oct. 25, 2012, 10:51 p.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7730/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2012, 10:51 p.m.)
>
>
> Review request for zookeeper, Patrick Hunt and Ted Yu.
>
>
> Description
> -------
>
> see ZOOKEEPER-1560 JIRA
>
>
> This addresses bug ZOOKEEPER-1560.
> https://issues.apache.org/jira/browse/ZOOKEEPER-1560
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 70d8538
>
> Diff: https://reviews.apache.org/r/7730/diff/
>
>
> Testing
> -------
>
> unit tests (including testLargeNodeData from ZOOKEEPER-1560 JIRA)
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>
Re: Review Request: patch for ZOOKEEPER-1560: Zookeeper client hangs on
creation of large nodes
Posted by Skye Wanderman-Milne <sk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7730/#review12800
-----------------------------------------------------------
Looks like I'm no longer passing unit tests, please hold off on further reviews for now...
- Skye Wanderman-Milne
On Oct. 25, 2012, 10:51 p.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7730/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2012, 10:51 p.m.)
>
>
> Review request for zookeeper, Patrick Hunt and Ted Yu.
>
>
> Description
> -------
>
> see ZOOKEEPER-1560 JIRA
>
>
> This addresses bug ZOOKEEPER-1560.
> https://issues.apache.org/jira/browse/ZOOKEEPER-1560
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 70d8538
>
> Diff: https://reviews.apache.org/r/7730/diff/
>
>
> Testing
> -------
>
> unit tests (including testLargeNodeData from ZOOKEEPER-1560 JIRA)
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>
Re: Review Request: patch for ZOOKEEPER-1560: Zookeeper client hangs on
creation of large nodes
Posted by Ted Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7730/#review12796
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java
<https://reviews.apache.org/r/7730/#comment27386>
'starting' -> 'started'
- Ted Yu
On Oct. 25, 2012, 10:51 p.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7730/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2012, 10:51 p.m.)
>
>
> Review request for zookeeper, Patrick Hunt and Ted Yu.
>
>
> Description
> -------
>
> see ZOOKEEPER-1560 JIRA
>
>
> This addresses bug ZOOKEEPER-1560.
> https://issues.apache.org/jira/browse/ZOOKEEPER-1560
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 70d8538
>
> Diff: https://reviews.apache.org/r/7730/diff/
>
>
> Testing
> -------
>
> unit tests (including testLargeNodeData from ZOOKEEPER-1560 JIRA)
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>
Re: Review Request: patch for ZOOKEEPER-1560: Zookeeper client hangs on
creation of large nodes
Posted by Skye Wanderman-Milne <sk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7730/
-----------------------------------------------------------
(Updated Oct. 25, 2012, 10:51 p.m.)
Review request for zookeeper, Patrick Hunt and Ted Yu.
Changes
-------
Messed up findSendablePacket logic, oops
Description
-------
see ZOOKEEPER-1560 JIRA
This addresses bug ZOOKEEPER-1560.
https://issues.apache.org/jira/browse/ZOOKEEPER-1560
Diffs (updated)
-----
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 70d8538
Diff: https://reviews.apache.org/r/7730/diff/
Testing
-------
unit tests (including testLargeNodeData from ZOOKEEPER-1560 JIRA)
Thanks,
Skye Wanderman-Milne
Re: Review Request: patch for ZOOKEEPER-1560: Zookeeper client hangs on
creation of large nodes
Posted by Skye Wanderman-Milne <sk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7730/
-----------------------------------------------------------
(Updated Oct. 25, 2012, 10:38 p.m.)
Review request for zookeeper, Patrick Hunt and Ted Yu.
Description
-------
see ZOOKEEPER-1560 JIRA
This addresses bug ZOOKEEPER-1560.
https://issues.apache.org/jira/browse/ZOOKEEPER-1560
Diffs (updated)
-----
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 70d8538
Diff: https://reviews.apache.org/r/7730/diff/
Testing
-------
unit tests (including testLargeNodeData from ZOOKEEPER-1560 JIRA)
Thanks,
Skye Wanderman-Milne