> On 2011-09-27 13:59:22, Camille Fournier wrote: > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, line 195 > > <https://reviews.apache.org/r/1714/diff/1/?file=37950#file37950line195> > > > > Why was this removed? It is not germane to the patch at hand. > > Thomas Koch wrote: > You're right, the removal had nothing to do with the issue, but it was > the right thing to do, once I've been there. It may happen in rare cases, > that connect returns true already there, but primeConnection() is called > anyways later from doTransport(). Having to possible call origins only adds > to uncertainty. > Did you observe any problems with this?
Are you sure that the lines in doTransport() will be called and executed if the socket connects immediately? I don't personally know whether that is the case or not. You didn't write any tests to show that this would happen, and you shouldn't have made the change for purposes of removing uncertainty without explicitly calling out what you were doing. If you are quite sure that even in the case of an immediate connection, OP_CONNECT will be set and we will hit that in doTransport, I'm fine with this change. But I don't like having changes like this snuck in under the radar, because if this is not the case and it causes a bug, it will be a massive pain to find. - Camille ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1714/#review2090 ----------------------------------------------------------- On 2011-09-14 22:18:24, Thomas Koch wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1714/ > ----------------------------------------------------------- > > (Updated 2011-09-14 22:18:24) > > > Review request for zookeeper. > > > Summary > ------- > > . > > > This addresses bug ZOOKEEPER-786. > https://issues.apache.org/jira/browse/ZOOKEEPER-786 > > > Diffs > ----- > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 626da04 > > Diff: https://reviews.apache.org/r/1714/diff > > > Testing > ------- > > > Thanks, > > Thomas > >
