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