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