You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by fp...@apache.org on 2014/11/01 13:14:31 UTC
Re: Review Request 27244: ZOOKEEPER-2069
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review59363
-----------------------------------------------------------
First batch of comments. The overall structure looks good, but I have a few questions and comments.
src/java/main/org/apache/zookeeper/ClientCnxnSocket.java
<https://reviews.apache.org/r/27244/#comment100641>
"... already started sending...". Also, the comment is overflowing, so moving it to the top sounds like a good choice.
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment100745>
Shouldn't isConnected return true in the case channel isn't null instead of channelFactory? In fact, it sounds like this implementation is instantiating a new ChannelFactory every time it tries to connect (modulo an attempt already being in progress). It doesn't sound necessary.
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment100747>
Could you use the future object it returns as opposed to using the firstConnect CountDownLatch? I think you're using the latch to determine if the connection request is still pending.
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment100746>
A message like "outgoingQueue isn't empty, but we haven't been notified". Also, I'm not sure how this could happen, is this a potential race or just a sanity check?
- fpj
On Oct. 31, 2014, 10:57 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2014, 10:57 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07
> src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 5ca0ba7
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java adb27ee
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION
> src/java/main/org/apache/zookeeper/ZooKeeper.java dd13cc9
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteBase.java PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteHammerTest.java PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/27244/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Nov. 1, 2014, 12:14 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocket.java, line 154
> > <https://reviews.apache.org/r/27244/diff/5/?file=745331#file745331line154>
> >
> > "... already started sending...". Also, the comment is overflowing, so moving it to the top sounds like a good choice.
This is old code. I only make this a common static func.
I will fix it.
> On Nov. 1, 2014, 12:14 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 66
> > <https://reviews.apache.org/r/27244/diff/6/?file=746460#file746460line66>
> >
> > Shouldn't isConnected return true in the case channel isn't null instead of channelFactory? In fact, it sounds like this implementation is instantiating a new ChannelFactory every time it tries to connect (modulo an attempt already being in progress). It doesn't sound necessary.
I thought the same thing. Here I am writing as similar code as I could to ZOOKEEPER-723 (Patrick's previous patch). I will change to use `channel` instead. Thanks!
> On Nov. 1, 2014, 12:14 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 88
> > <https://reviews.apache.org/r/27244/diff/6/?file=746460#file746460line88>
> >
> > Could you use the future object it returns as opposed to using the firstConnect CountDownLatch? I think you're using the latch to determine if the connection request is still pending.
It needs to do some initialization work before entering doTransportation. I am wondering if that `Future` object from connect could ensure that. I have asked a question to netty dev and waiting for response.
> On Nov. 1, 2014, 12:14 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 165
> > <https://reviews.apache.org/r/27244/diff/6/?file=746460#file746460line165>
> >
> > A message like "outgoingQueue isn't empty, but we haven't been notified". Also, I'm not sure how this could happen, is this a potential race or just a sanity check?
It's something I am not sure about. I am worried that I might dismiss some places to wake it up. As the comment stated, *last straw*.
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review59363
-----------------------------------------------------------
On Oct. 31, 2014, 10:57 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2014, 10:57 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07
> src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 5ca0ba7
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java adb27ee
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION
> src/java/main/org/apache/zookeeper/ZooKeeper.java dd13cc9
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteBase.java PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteHammerTest.java PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/27244/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>