You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Hongchao Deng <hd...@cloudera.com> on 2014/10/27 22:23:25 UTC
Review Request 27244: ZOOKEEPER-2069
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
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/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. 21, 2014, 7:46 p.m., Raul Gutierrez Segales wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocket.java, line 179
> > <https://reviews.apache.org/r/27244/diff/20/?file=758279#file758279line179>
> >
> > nit:
> >
> > * extrapolation reads better than concatenation
> > * p.toString() is cheap, so i don't think the isDebugEnabled() fencing is needed here
Not getting your point. Would you write down the code here?
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review62605
-----------------------------------------------------------
On Nov. 11, 2014, 8:54 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2014, 8:54 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 Raul Gutierrez Segales <rg...@itevenworks.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review62605
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/ClientCnxnSocket.java
<https://reviews.apache.org/r/27244/#comment104673>
nit: it's implicit that you are talking about the client and clientTunneledAuthenticationInProgress is way too long, how about:
tunneledAuthInProgres
?
Or maybe something even shorter if you have a better idea.
src/java/main/org/apache/zookeeper/ClientCnxnSocket.java
<https://reviews.apache.org/r/27244/#comment104674>
nit: style, i think we always put the || operator at the end of the expression, i.e.:
```
if (outgoingQueue.getFirst().bb != null ||
!tunneledAuthInProgress) {
}
```
src/java/main/org/apache/zookeeper/ClientCnxnSocket.java
<https://reviews.apache.org/r/27244/#comment104677>
nit:
* extrapolation reads better than concatenation
* p.toString() is cheap, so i don't think the isDebugEnabled() fencing is needed here
src/java/main/org/apache/zookeeper/ClientCnxnSocket.java
<https://reviews.apache.org/r/27244/#comment104675>
nit: i'd drop this comment
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment104679>
nit, simpler message:
```
throw new IOException("Channel exists: duplicate connect?");
```
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment104681>
nit: code is straightforward, i'd drop the comment
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment104683>
nit: newline
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment104684>
nit: drop the comment
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment104687>
we are already in a file called netty, lets just call this method cleanup().
- Raul Gutierrez Segales
On Nov. 11, 2014, 8:54 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2014, 8:54 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. 24, 2014, 6:53 p.m., Raul Gutierrez Segales wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocket.java, line 179
> > <https://reviews.apache.org/r/27244/diff/20/?file=758279#file758279line179>
> >
> > By extraplation I meant that this:
> >
> > ```
> > LOG.debug("deferring non-priming packet {} until SASL authentation completes.", p);
> > ```
> >
> > reads nicer than using +.
> >
> > I also think you can drop the if (LOG.isDebugEnabled()) fencing entirely, since it only makes sense when it's very expensive to generate the debug message (which, I think, isn't the case here).
This was part of the legacy code that I moved from NIO. Anyway, time to fix and make it better :)
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review62842
-----------------------------------------------------------
On Nov. 22, 2014, 5:46 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2014, 5:46 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 Raul Gutierrez Segales <rg...@itevenworks.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review62842
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/ClientCnxnSocket.java
<https://reviews.apache.org/r/27244/#comment104972>
By extraplation I meant that this:
```
LOG.debug("deferring non-priming packet {} until SASL authentation completes.", p);
```
reads nicer than using +.
I also think you can drop the if (LOG.isDebugEnabled()) fencing entirely, since it only makes sense when it's very expensive to generate the debug message (which, I think, isn't the case here).
- Raul Gutierrez Segales
On Nov. 22, 2014, 5:46 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2014, 5:46 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 fp...@apache.org.
> On Dec. 11, 2014, 11:16 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 134
> > <https://reviews.apache.org/r/27244/diff/28/?file=789547#file789547line134>
> >
> > It sounds like making pendingQueue and outgoingQueue are optimizations that are independent from this issue. Shouldn't we make this change in a different jira?
>
> Hongchao Deng wrote:
> outgoingQueue isn't optimization... My previous patch used a semaphore which will be waken up by WakeupCnxn(). Your suggestion is use a blocking outgoingQueue. What do you think? I am fine since I know both changes well and I can do it in separate JIRAs.
>
> fpj wrote:
> My concern here is that it ended up implying more changes than just removing the semaphore. For example, the patch removes this synchronized block (not sure it is going to format it right):
>
> for (Packet p : outgoingQueue) {
> conLossPacket(p);
> }
> outgoingQueue.clear();
>
> and checking the other synchronized blocks removed, it isn't entirely straightforward that you can remove all of them.
>
> Hongchao Deng wrote:
> I can add those synchronized back. However, findbugs will complain that so I will add rules too.
>
> On Netty side I can assure your concern:
> 1. In ClientCnxn, primeConnection(), cleanup(), queuePacket(). primeConnection(), cleanup() is guaranteed to be start and end of life cycles. queuePacket() is appending packets to outgoingQueue and waking up the blocking queue.
>
>
> What do you think?
I'm not suggesting that you bring back the synchronized blocks. I'm saying that I haven't checked carefully and it isn't straightforward that you can remove them. For example, the one in ClientCnxnSocketNIO line 163 has multiple accesses to outgoingQueue, so removing the synchronized block doesn't necessarily gives us the same behavior. Can you actually justify the removal of each one of those synchronized blocks? If you aren't sure, then I'll check it carefully.
- fpj
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review64807
-----------------------------------------------------------
On Dec. 12, 2014, 7:16 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2014, 7:16 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> 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/ZooKeeperTestable.java 775d1a2
> src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
> src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
> 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
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Dec. 11, 2014, 11:16 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 134
> > <https://reviews.apache.org/r/27244/diff/28/?file=789547#file789547line134>
> >
> > It sounds like making pendingQueue and outgoingQueue are optimizations that are independent from this issue. Shouldn't we make this change in a different jira?
outgoingQueue isn't optimization... My previous patch used a semaphore which will be waken up by WakeupCnxn(). Your suggestion is use a blocking outgoingQueue. What do you think? I am fine since I know both changes well and I can do it in separate JIRAs.
> On Dec. 11, 2014, 11:16 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 1472
> > <https://reviews.apache.org/r/27244/diff/28/?file=789547#file789547line1472>
> >
> > I'm now confused by this change. Are you getting rid of enableWrite? saslCompleted is fairly different from enableWrite.
So the guy who make the SASL change do it in a hackish way. It simply blocks the entire NIO thread and send it directly through socket. But when it finishes SASL, it calls enableWrite(). I can definitely use the old name enableWrite(). But do you think it is better to drive the change here? Since Netty client doesn't need to enable write here.
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review64807
-----------------------------------------------------------
On Dec. 11, 2014, 6:11 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2014, 6:11 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> 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/ZooKeeperTestable.java 775d1a2
> src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
> src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
> 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
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Dec. 11, 2014, 11:16 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 134
> > <https://reviews.apache.org/r/27244/diff/28/?file=789547#file789547line134>
> >
> > It sounds like making pendingQueue and outgoingQueue are optimizations that are independent from this issue. Shouldn't we make this change in a different jira?
>
> Hongchao Deng wrote:
> outgoingQueue isn't optimization... My previous patch used a semaphore which will be waken up by WakeupCnxn(). Your suggestion is use a blocking outgoingQueue. What do you think? I am fine since I know both changes well and I can do it in separate JIRAs.
>
> fpj wrote:
> My concern here is that it ended up implying more changes than just removing the semaphore. For example, the patch removes this synchronized block (not sure it is going to format it right):
>
> for (Packet p : outgoingQueue) {
> conLossPacket(p);
> }
> outgoingQueue.clear();
>
> and checking the other synchronized blocks removed, it isn't entirely straightforward that you can remove all of them.
>
> Hongchao Deng wrote:
> I can add those synchronized back. However, findbugs will complain that so I will add rules too.
>
> On Netty side I can assure your concern:
> 1. In ClientCnxn, primeConnection(), cleanup(), queuePacket(). primeConnection(), cleanup() is guaranteed to be start and end of life cycles. queuePacket() is appending packets to outgoingQueue and waking up the blocking queue.
>
>
> What do you think?
>
> fpj wrote:
> I'm not suggesting that you bring back the synchronized blocks. I'm saying that I haven't checked carefully and it isn't straightforward that you can remove them. For example, the one in ClientCnxnSocketNIO line 163 has multiple accesses to outgoingQueue, so removing the synchronized block doesn't necessarily gives us the same behavior. Can you actually justify the removal of each one of those synchronized blocks? If you aren't sure, then I'll check it carefully.
That would be great if you can check it. Actually I just found a possible race.
Here's my version of analysis:
1. primeConnection: The start of life cycle. No one else can touch outgoingQueue except queuePacket(). primeConn prepends packets while queuePacket appends.
2. !!cleanup: The end of life cycle. No one else except queuePacket(). A possible race with queuePacket() in check closing and add packets. I will fix it shortly.
3. queuePacket: ...
4. findSendablePacket and doIO: this is the only place that remove items from outgoing Queue
- the check in findSendablePacket() guarantees not empty. Because no where will remove items
- enumeration will give a consistent current view. Adding new packets doesn't count in.
- found non-priming packet, remove and addFirst wouldn't have race because here only queuePacket() will be called. And queuePacket() appends only.
5. last check in doTransport: this is duplicate..
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review64807
-----------------------------------------------------------
On Dec. 12, 2014, 7:16 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2014, 7:16 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> 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/ZooKeeperTestable.java 775d1a2
> src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
> src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
> 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
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Dec. 11, 2014, 11:16 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 1472
> > <https://reviews.apache.org/r/27244/diff/28/?file=789547#file789547line1472>
> >
> > I'm now confused by this change. Are you getting rid of enableWrite? saslCompleted is fairly different from enableWrite.
>
> Hongchao Deng wrote:
> So the guy who make the SASL change do it in a hackish way. It simply blocks the entire NIO thread and send it directly through socket. But when it finishes SASL, it calls enableWrite(). I can definitely use the old name enableWrite(). But do you think it is better to drive the change here? Since Netty client doesn't need to enable write here.
Hi Flavio. Maybe you mean enableWrite() is also used by CilentCnxnNIO and worried that I change the name as well. I only change the interface to outside. Inside ClientCnxnNIO there is still a enableWrite function which is used inside the class's other places.
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review64807
-----------------------------------------------------------
On Dec. 11, 2014, 6:11 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2014, 6:11 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> 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/ZooKeeperTestable.java 775d1a2
> src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
> src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
> 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
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Dec. 11, 2014, 11:16 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 134
> > <https://reviews.apache.org/r/27244/diff/28/?file=789547#file789547line134>
> >
> > It sounds like making pendingQueue and outgoingQueue are optimizations that are independent from this issue. Shouldn't we make this change in a different jira?
>
> Hongchao Deng wrote:
> outgoingQueue isn't optimization... My previous patch used a semaphore which will be waken up by WakeupCnxn(). Your suggestion is use a blocking outgoingQueue. What do you think? I am fine since I know both changes well and I can do it in separate JIRAs.
>
> fpj wrote:
> My concern here is that it ended up implying more changes than just removing the semaphore. For example, the patch removes this synchronized block (not sure it is going to format it right):
>
> for (Packet p : outgoingQueue) {
> conLossPacket(p);
> }
> outgoingQueue.clear();
>
> and checking the other synchronized blocks removed, it isn't entirely straightforward that you can remove all of them.
>
> Hongchao Deng wrote:
> I can add those synchronized back. However, findbugs will complain that so I will add rules too.
>
> On Netty side I can assure your concern:
> 1. In ClientCnxn, primeConnection(), cleanup(), queuePacket(). primeConnection(), cleanup() is guaranteed to be start and end of life cycles. queuePacket() is appending packets to outgoingQueue and waking up the blocking queue.
>
>
> What do you think?
>
> fpj wrote:
> I'm not suggesting that you bring back the synchronized blocks. I'm saying that I haven't checked carefully and it isn't straightforward that you can remove them. For example, the one in ClientCnxnSocketNIO line 163 has multiple accesses to outgoingQueue, so removing the synchronized block doesn't necessarily gives us the same behavior. Can you actually justify the removal of each one of those synchronized blocks? If you aren't sure, then I'll check it carefully.
>
> Hongchao Deng wrote:
> That would be great if you can check it. Actually I just found a possible race.
>
> Here's my version of analysis:
> 1. primeConnection: The start of life cycle. No one else can touch outgoingQueue except queuePacket(). primeConn prepends packets while queuePacket appends.
> 2. !!cleanup: The end of life cycle. No one else except queuePacket(). A possible race with queuePacket() in check closing and add packets. I will fix it shortly.
> 3. queuePacket: ...
> 4. findSendablePacket and doIO: this is the only place that remove items from outgoing Queue
> - the check in findSendablePacket() guarantees not empty. Because no where will remove items
> - enumeration will give a consistent current view. Adding new packets doesn't count in.
> - found non-priming packet, remove and addFirst wouldn't have race because here only queuePacket() will be called. And queuePacket() appends only.
> 5. last check in doTransport: this is duplicate..
I just realize between cleanup() and queuePacket() there is race regardless of the synchronized..
Even though cleanup() finishes, it doesn't guarantee that state is CLOSED or closeSession packet was sent. Thus queuePacket() doesn't call conLossPacket().
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review64807
-----------------------------------------------------------
On Dec. 12, 2014, 7:16 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2014, 7:16 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> 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/ZooKeeperTestable.java 775d1a2
> src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
> src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
> 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
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Dec. 11, 2014, 11:16 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 134
> > <https://reviews.apache.org/r/27244/diff/28/?file=789547#file789547line134>
> >
> > It sounds like making pendingQueue and outgoingQueue are optimizations that are independent from this issue. Shouldn't we make this change in a different jira?
>
> Hongchao Deng wrote:
> outgoingQueue isn't optimization... My previous patch used a semaphore which will be waken up by WakeupCnxn(). Your suggestion is use a blocking outgoingQueue. What do you think? I am fine since I know both changes well and I can do it in separate JIRAs.
>
> fpj wrote:
> My concern here is that it ended up implying more changes than just removing the semaphore. For example, the patch removes this synchronized block (not sure it is going to format it right):
>
> for (Packet p : outgoingQueue) {
> conLossPacket(p);
> }
> outgoingQueue.clear();
>
> and checking the other synchronized blocks removed, it isn't entirely straightforward that you can remove all of them.
I can add those synchronized back. However, findbugs will complain that so I will add rules too.
On Netty side I can assure your concern:
1. In ClientCnxn, primeConnection(), cleanup(), queuePacket(). primeConnection(), cleanup() is guaranteed to be start and end of life cycles. queuePacket() is appending packets to outgoingQueue and waking up the blocking queue.
What do you think?
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review64807
-----------------------------------------------------------
On Dec. 12, 2014, 7:16 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2014, 7:16 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> 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/ZooKeeperTestable.java 775d1a2
> src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
> src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
> 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
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by fp...@apache.org.
> On Dec. 11, 2014, 11:16 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 1472
> > <https://reviews.apache.org/r/27244/diff/28/?file=789547#file789547line1472>
> >
> > I'm now confused by this change. Are you getting rid of enableWrite? saslCompleted is fairly different from enableWrite.
>
> Hongchao Deng wrote:
> So the guy who make the SASL change do it in a hackish way. It simply blocks the entire NIO thread and send it directly through socket. But when it finishes SASL, it calls enableWrite(). I can definitely use the old name enableWrite(). But do you think it is better to drive the change here? Since Netty client doesn't need to enable write here.
>
> Hongchao Deng wrote:
> Hi Flavio. Maybe you mean enableWrite() is also used by CilentCnxnNIO and worried that I change the name as well. I only change the interface to outside. Inside ClientCnxnNIO there is still a enableWrite function which is used inside the class's other places.
Yes, you're right, I did confuse the calls.
> On Dec. 11, 2014, 11:16 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 134
> > <https://reviews.apache.org/r/27244/diff/28/?file=789547#file789547line134>
> >
> > It sounds like making pendingQueue and outgoingQueue are optimizations that are independent from this issue. Shouldn't we make this change in a different jira?
>
> Hongchao Deng wrote:
> outgoingQueue isn't optimization... My previous patch used a semaphore which will be waken up by WakeupCnxn(). Your suggestion is use a blocking outgoingQueue. What do you think? I am fine since I know both changes well and I can do it in separate JIRAs.
My concern here is that it ended up implying more changes than just removing the semaphore. For example, the patch removes this synchronized block (not sure it is going to format it right):
for (Packet p : outgoingQueue) {
conLossPacket(p);
}
outgoingQueue.clear();
and checking the other synchronized blocks removed, it isn't entirely straightforward that you can remove all of them.
- fpj
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review64807
-----------------------------------------------------------
On Dec. 12, 2014, 7:16 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2014, 7:16 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> 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/ZooKeeperTestable.java 775d1a2
> src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
> src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
> 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
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by fp...@apache.org.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review64807
-----------------------------------------------------------
A few more, I'm not sorry for sending them separately.
src/java/main/org/apache/zookeeper/ClientCnxn.java
<https://reviews.apache.org/r/27244/#comment107583>
It sounds like making pendingQueue and outgoingQueue are optimizations that are independent from this issue. Shouldn't we make this change in a different jira?
src/java/main/org/apache/zookeeper/ClientCnxn.java
<https://reviews.apache.org/r/27244/#comment107584>
I'm now confused by this change. Are you getting rid of enableWrite? saslCompleted is fairly different from enableWrite.
src/java/test/org/apache/zookeeper/test/ClientTest.java
<https://reviews.apache.org/r/27244/#comment107585>
Good catch.
- fpj
On Dec. 11, 2014, 6:11 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2014, 6:11 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> 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/ZooKeeperTestable.java 775d1a2
> src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
> src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
> 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
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Dec. 17, 2014, 11:30 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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/ZooKeeperTestable.java 775d1a2
src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
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
-------
1. use LinkedBlockingDeque.
Thanks,
Hongchao Deng
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Dec. 13, 2014, 11:05 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
build.xml bb5ff4f
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/ZooKeeperTestable.java 775d1a2
src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
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
-------
1. use LinkedBlockingDeque.
Thanks,
Hongchao Deng
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Dec. 12, 2014, 7:16 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
build.xml bb5ff4f
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/ZooKeeperTestable.java 775d1a2
src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
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
-------
1. use LinkedBlockingDeque.
Thanks,
Hongchao Deng
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Dec. 12, 2014, 12:14 a.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
build.xml bb5ff4f
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/ZooKeeperTestable.java 775d1a2
src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
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
-------
1. use LinkedBlockingDeque.
Thanks,
Hongchao Deng
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Dec. 11, 2014, 7:37 p.m., Rakesh R wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 99
> > <https://reviews.apache.org/r/27244/diff/28/?file=789550#file789550line99>
> >
> > Could you tell me when this situation can come? Is this added for safer side?
I will remove it. I had this code for safe check when I didn't know ClientCnxn so well.
> On Dec. 11, 2014, 7:37 p.m., Rakesh R wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 63
> > <https://reviews.apache.org/r/27244/diff/28/?file=789550#file789550line63>
> >
> > Can you give thread name for this thread pool like,
> >
> > Executors.newCachedThreadPool(new ThreadFactoryBuilder() .setNameFormat("ZooKeeper-NIOBoss-%d").build()), Executors.newCachedThreadPool(new ThreadFactoryBuilder() .setNameFormat("ZooKeeper-NIOWorker-%d").build())
ThreadFactoryBuilder is from Guava... We didn't have it yet.
> On Dec. 11, 2014, 7:37 p.m., Rakesh R wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 108
> > <https://reviews.apache.org/r/27244/diff/28/?file=789550#file789550line108>
> >
> > bootstrap.setOption("connectTimeoutMillis", System.getProperty("connectTimeoutMillis"));
> >
> > What do you think about providing 'clientConnectTimeoutMillis' as a configurable item. By default its 10secs.
Sounds good. But currently the timeout thing is handled outside in ClientCnxn sendThread. This SSL thing will gradually drive transformation to Netty style connection :)
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review64767
-----------------------------------------------------------
On Dec. 11, 2014, 6:11 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2014, 6:11 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> 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/ZooKeeperTestable.java 775d1a2
> src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
> src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
> 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
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Rakesh R <ra...@huawei.com>.
> On Dec. 11, 2014, 7:37 p.m., Rakesh R wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 108
> > <https://reviews.apache.org/r/27244/diff/28/?file=789550#file789550line108>
> >
> > bootstrap.setOption("connectTimeoutMillis", System.getProperty("connectTimeoutMillis"));
> >
> > What do you think about providing 'clientConnectTimeoutMillis' as a configurable item. By default its 10secs.
>
> Hongchao Deng wrote:
> Sounds good. But currently the timeout thing is handled outside in ClientCnxn sendThread. This SSL thing will gradually drive transformation to Netty style connection :)
ok. agreed.
> On Dec. 11, 2014, 7:37 p.m., Rakesh R wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 99
> > <https://reviews.apache.org/r/27244/diff/28/?file=789550#file789550line99>
> >
> > Could you tell me when this situation can come? Is this added for safer side?
>
> Hongchao Deng wrote:
> I will remove it. I had this code for safe check when I didn't know ClientCnxn so well.
ok. fine
> On Dec. 11, 2014, 7:37 p.m., Rakesh R wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 63
> > <https://reviews.apache.org/r/27244/diff/28/?file=789550#file789550line63>
> >
> > Can you give thread name for this thread pool like,
> >
> > Executors.newCachedThreadPool(new ThreadFactoryBuilder() .setNameFormat("ZooKeeper-NIOBoss-%d").build()), Executors.newCachedThreadPool(new ThreadFactoryBuilder() .setNameFormat("ZooKeeper-NIOWorker-%d").build())
>
> Hongchao Deng wrote:
> ThreadFactoryBuilder is from Guava... We didn't have it yet.
oh yes. Good if we can name the thread, otw leave it:)
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review64767
-----------------------------------------------------------
On Dec. 12, 2014, 12:14 a.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2014, 12:14 a.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> 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/ZooKeeperTestable.java 775d1a2
> src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
> src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
> 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
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Rakesh R <ra...@huawei.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review64767
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment107547>
Can you give thread name for this thread pool like,
Executors.newCachedThreadPool(new ThreadFactoryBuilder() .setNameFormat("ZooKeeper-NIOBoss-%d").build()), Executors.newCachedThreadPool(new ThreadFactoryBuilder() .setNameFormat("ZooKeeper-NIOWorker-%d").build())
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment107544>
Can we make the visibiliy of the member variables to private.
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment107543>
Could you tell me when this situation can come? Is this added for safer side?
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment107526>
bootstrap.setOption("connectTimeoutMillis", System.getProperty("connectTimeoutMillis"));
What do you think about providing 'clientConnectTimeoutMillis' as a configurable item. By default its 10secs.
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment107527>
Before return add log msg, it will help in debugging/analysing failure cases. Also, when loggin get the cause from the future object.
future.getCause()
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment107531>
During logging it can log the channel details like
channelFuture.getChannel(), consider multiple zkclients running in same JVM. The same comment is applicable wherever doing channel related logs.
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment107533>
Make it private static class WakeupPacket
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment107528>
Add channel details in the log like,
LOG.info("channel {} is disconnected", ctx.getChannel());
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment107535>
use {} instead of +
- Rakesh R
On Dec. 11, 2014, 6:11 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2014, 6:11 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> 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/ZooKeeperTestable.java 775d1a2
> src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
> src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
> 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
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by fp...@apache.org.
> On Dec. 11, 2014, 10:53 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 70
> > <https://reviews.apache.org/r/27244/diff/28/?file=789550#file789550line70>
> >
> > I see, the previous sasl changes are necessary here so that we can proceed only if authentication suceeds. Please confirm.
>
> Hongchao Deng wrote:
> I change enableWrite(), enableReadWriteOnly(), WakeupCnxn() interfaces:
> 1. enable read, write is renaming only. Because in netty it doesn't make sense to say enable write. It's more natural to say SASL completed and what we are doing here.
> 2. I split WakeupCnxn() into two because they are for two purposes: one is for notifying packet has been added to outgoingQueue, the other is for notifying connection is intentionally closed. They are different in Netty.
Ok, got it. Netty 3 actually allowed you to control those flags directly, but apparently they got rid of it in Netty 4. See this http://stackoverflow.com/questions/24888912/where-is-channel-setinterestops-in-netty-4x
I think you're saying that the enableWrite call was there only for sasl so you prefer to just name explicitly? If that's the intention, then seems good to me.
> On Dec. 11, 2014, 10:53 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, line 68
> > <https://reviews.apache.org/r/27244/diff/28/?file=789549#file789549line68>
> >
> > Why does pendingQueue need to be made concurrent?
>
> Hongchao Deng wrote:
> I will convert it back.
If it is worth having this optmization, then I suggest you propose it in another jira. You have the changes already, it should be easy to generate a patch.
> On Dec. 11, 2014, 10:53 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 1372
> > <https://reviews.apache.org/r/27244/diff/28/?file=789547#file789547line1372>
> >
> > If these SASL changes aren't strictly necessary for this patch, we should do them in a different jira.
>
> Hongchao Deng wrote:
> This is commented by Raul @rgs to change.. I usually didn't change name myself.
My question wasn't about the name change, but instead about the changes around sasl. I got what you're trying to do now.
- fpj
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review64803
-----------------------------------------------------------
On Dec. 11, 2014, 6:11 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2014, 6:11 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> 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/ZooKeeperTestable.java 775d1a2
> src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
> src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
> 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
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Dec. 11, 2014, 10:53 p.m., fpj wrote:
> > build.xml, line 42
> > <https://reviews.apache.org/r/27244/diff/28/?file=789546#file789546line42>
> >
> > Is this accidental? There is an issue open to change it to 1.7. If it is necessary, we can make this change here, but otherwise leave it for the other patch.
In order to get a blocking outgoing queue in with minimum change (as stated in your last comment), I choose to use LinkedBlockingDeque, which is required by JDK 6. Changing to 1.7 seems fine with me :)
> On Dec. 11, 2014, 10:53 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 1372
> > <https://reviews.apache.org/r/27244/diff/28/?file=789547#file789547line1372>
> >
> > If these SASL changes aren't strictly necessary for this patch, we should do them in a different jira.
This is commented by Raul @rgs to change.. I usually didn't change name myself.
> On Dec. 11, 2014, 10:53 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, line 68
> > <https://reviews.apache.org/r/27244/diff/28/?file=789549#file789549line68>
> >
> > Why does pendingQueue need to be made concurrent?
I will convert it back.
> On Dec. 11, 2014, 10:53 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 70
> > <https://reviews.apache.org/r/27244/diff/28/?file=789550#file789550line70>
> >
> > I see, the previous sasl changes are necessary here so that we can proceed only if authentication suceeds. Please confirm.
I change enableWrite(), enableReadWriteOnly(), WakeupCnxn() interfaces:
1. enable read, write is renaming only. Because in netty it doesn't make sense to say enable write. It's more natural to say SASL completed and what we are doing here.
2. I split WakeupCnxn() into two because they are for two purposes: one is for notifying packet has been added to outgoingQueue, the other is for notifying connection is intentionally closed. They are different in Netty.
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review64803
-----------------------------------------------------------
On Dec. 11, 2014, 6:11 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2014, 6:11 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> 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/ZooKeeperTestable.java 775d1a2
> src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
> src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
> 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
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by fp...@apache.org.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review64803
-----------------------------------------------------------
A few more comments, Hongchao. Thanks for the changes so far.
build.xml
<https://reviews.apache.org/r/27244/#comment107568>
Is this accidental? There is an issue open to change it to 1.7. If it is necessary, we can make this change here, but otherwise leave it for the other patch.
src/java/main/org/apache/zookeeper/ClientCnxn.java
<https://reviews.apache.org/r/27244/#comment107569>
If you end up generating a new patch, please remove this tab/spaces.
src/java/main/org/apache/zookeeper/ClientCnxn.java
<https://reviews.apache.org/r/27244/#comment107570>
If these SASL changes aren't strictly necessary for this patch, we should do them in a different jira.
src/java/main/org/apache/zookeeper/ClientCnxn.java
<https://reviews.apache.org/r/27244/#comment107571>
See previous comment.
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java
<https://reviews.apache.org/r/27244/#comment107572>
Why does pendingQueue need to be made concurrent?
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java
<https://reviews.apache.org/r/27244/#comment107573>
TODOs require a jira number.
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment107577>
I see, the previous sasl changes are necessary here so that we can proceed only if authentication suceeds. Please confirm.
- fpj
On Dec. 11, 2014, 6:11 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2014, 6:11 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> 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/ZooKeeperTestable.java 775d1a2
> src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
> src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
> 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
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Dec. 11, 2014, 6:11 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
build.xml bb5ff4f
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/ZooKeeperTestable.java 775d1a2
src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
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
-------
1. use LinkedBlockingDeque.
Thanks,
Hongchao Deng
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Dec. 6, 2014, 2:11 a.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
build.xml bb5ff4f
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/ZooKeeperTestable.java 775d1a2
src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
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
-------
1. use LinkedBlockingDeque.
Thanks,
Hongchao Deng
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Dec. 5, 2014, 10:35 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
build.xml bb5ff4f
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/main/org/apache/zookeeper/ZooKeeperTestable.java 775d1a2
src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
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
-------
1. use LinkedBlockingDeque.
Thanks,
Hongchao Deng
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Dec. 3, 2014, 10:20 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
build.xml bb5ff4f
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
-------
1. use LinkedBlockingDeque.
Thanks,
Hongchao Deng
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Dec. 3, 2014, 9:50 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
build.xml bb5ff4f
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 (updated)
-------
1. use LinkedBlockingDeque.
Thanks,
Hongchao Deng
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Dec. 3, 2014, 1:52 a.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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/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 (updated)
-------
1. use ChannelFuture returned by connect()
2. remove NIOLock
Thanks,
Hongchao Deng
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 66
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line66>
> >
> > Is the reason to have workSemaphore to block in doTransport when there isn't anything in the outgoing queue? If so, we might be better off with a blocking queue rather than a list as we have right now.
Sounds good on my side. I tried to make minimal changes. Do you think I should make it in here or create another JIRA?
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 107
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line107>
> >
> > I'm not sure why you're not using the ChannelFuture returned here. See this connect() method in bookkeeper for an example https://github.com/apache/bookkeeper/blob/branch-4.0/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java#L112
Good. Fix it next
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 188
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line188>
> >
> > Could you elaborate on what you're trying to do with this NIOLock? I'm not sure what you mean with race cocnditions in the comment.
1. The intuition is that in NIO client all reads and writes are sequential.
2. One problem I found: (1) packet is removed from outgoingQueue; (2) before the packet is added to pendingQueue; (3) sendThread.readResponse throws exception.
3. I am wondering what the purpose of pendingQueue is except for checking and if we can remove it. AFAICT that's the only concurrency concern.
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review63604
-----------------------------------------------------------
On Nov. 24, 2014, 7:01 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Nov. 24, 2014, 7:01 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/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 Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 188
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line188>
> >
> > Could you elaborate on what you're trying to do with this NIOLock? I'm not sure what you mean with race cocnditions in the comment.
>
> Hongchao Deng wrote:
> 1. The intuition is that in NIO client all reads and writes are sequential.
> 2. One problem I found: (1) packet is removed from outgoingQueue; (2) before the packet is added to pendingQueue; (3) sendThread.readResponse throws exception.
> 3. I am wondering what the purpose of pendingQueue is except for checking and if we can remove it. AFAICT that's the only concurrency concern.
I come up with a better way if the pendingQueue is the only concern. It looks like pendingQueue doesn't care about ordering. So I can add packets to pendingQueue before writing to channel.
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review63604
-----------------------------------------------------------
On Nov. 24, 2014, 7:01 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Nov. 24, 2014, 7:01 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/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 fp...@apache.org.
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 66
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line66>
> >
> > Is the reason to have workSemaphore to block in doTransport when there isn't anything in the outgoing queue? If so, we might be better off with a blocking queue rather than a list as we have right now.
>
> Hongchao Deng wrote:
> Sounds good on my side. I tried to make minimal changes. Do you think I should make it in here or create another JIRA?
I'd rather have it fixed here, but depends on how much more energy you have to put into this patch.
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 107
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line107>
> >
> > I'm not sure why you're not using the ChannelFuture returned here. See this connect() method in bookkeeper for an example https://github.com/apache/bookkeeper/blob/branch-4.0/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java#L112
>
> Hongchao Deng wrote:
> Good. Fix it next
Ok.
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 188
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line188>
> >
> > Could you elaborate on what you're trying to do with this NIOLock? I'm not sure what you mean with race cocnditions in the comment.
>
> Hongchao Deng wrote:
> 1. The intuition is that in NIO client all reads and writes are sequential.
> 2. One problem I found: (1) packet is removed from outgoingQueue; (2) before the packet is added to pendingQueue; (3) sendThread.readResponse throws exception.
> 3. I am wondering what the purpose of pendingQueue is except for checking and if we can remove it. AFAICT that's the only concurrency concern.
>
> Hongchao Deng wrote:
> I come up with a better way if the pendingQueue is the only concern. It looks like pendingQueue doesn't care about ordering. So I can add packets to pendingQueue before writing to channel.
Check SendThread.readResponse() in ClientCnxn, pls.
- fpj
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review63604
-----------------------------------------------------------
On Dec. 3, 2014, 1:52 a.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2014, 1:52 a.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/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
> -------
>
> 1. use ChannelFuture returned by connect()
> 2. remove NIOLock
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 66
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line66>
> >
> > Is the reason to have workSemaphore to block in doTransport when there isn't anything in the outgoing queue? If so, we might be better off with a blocking queue rather than a list as we have right now.
>
> Hongchao Deng wrote:
> Sounds good on my side. I tried to make minimal changes. Do you think I should make it in here or create another JIRA?
>
> fpj wrote:
> I'd rather have it fixed here, but depends on how much more energy you have to put into this patch.
I get a new patch written. Tested in local jenkins.
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 188
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line188>
> >
> > Could you elaborate on what you're trying to do with this NIOLock? I'm not sure what you mean with race cocnditions in the comment.
>
> Hongchao Deng wrote:
> 1. The intuition is that in NIO client all reads and writes are sequential.
> 2. One problem I found: (1) packet is removed from outgoingQueue; (2) before the packet is added to pendingQueue; (3) sendThread.readResponse throws exception.
> 3. I am wondering what the purpose of pendingQueue is except for checking and if we can remove it. AFAICT that's the only concurrency concern.
>
> Hongchao Deng wrote:
> I come up with a better way if the pendingQueue is the only concern. It looks like pendingQueue doesn't care about ordering. So I can add packets to pendingQueue before writing to channel.
>
> fpj wrote:
> Check SendThread.readResponse() in ClientCnxn, pls.
I did. I figure out it holds the assumptions:
1. pendingQueue only takes non-priming packets.
2. pendingQueue ordering is the same as packets sent in.
I would re-clarify my point that I can remove the lock as long as the only interaction between read and write is the pendingQueue. :)
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review63604
-----------------------------------------------------------
On Dec. 3, 2014, 9:50 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2014, 9:50 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> 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
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 188
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line188>
> >
> > Could you elaborate on what you're trying to do with this NIOLock? I'm not sure what you mean with race cocnditions in the comment.
>
> Hongchao Deng wrote:
> 1. The intuition is that in NIO client all reads and writes are sequential.
> 2. One problem I found: (1) packet is removed from outgoingQueue; (2) before the packet is added to pendingQueue; (3) sendThread.readResponse throws exception.
> 3. I am wondering what the purpose of pendingQueue is except for checking and if we can remove it. AFAICT that's the only concurrency concern.
>
> Hongchao Deng wrote:
> I come up with a better way if the pendingQueue is the only concern. It looks like pendingQueue doesn't care about ordering. So I can add packets to pendingQueue before writing to channel.
>
> fpj wrote:
> Check SendThread.readResponse() in ClientCnxn, pls.
>
> Hongchao Deng wrote:
> I did. I figure out it holds the assumptions:
> 1. pendingQueue only takes non-priming packets.
> 2. pendingQueue ordering is the same as packets sent in.
>
> I would re-clarify my point that I can remove the lock as long as the only interaction between read and write is the pendingQueue. :)
>
> fpj wrote:
> I've been revisiting the code and I can't think of any other potential problem.
Cool.
I think my new patch is making too many changes. I will change the last thing you pointed out and update them only. Thanks!
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review63604
-----------------------------------------------------------
On Dec. 3, 2014, 10:20 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2014, 10:20 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> 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
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by fp...@apache.org.
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 188
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line188>
> >
> > Could you elaborate on what you're trying to do with this NIOLock? I'm not sure what you mean with race cocnditions in the comment.
>
> Hongchao Deng wrote:
> 1. The intuition is that in NIO client all reads and writes are sequential.
> 2. One problem I found: (1) packet is removed from outgoingQueue; (2) before the packet is added to pendingQueue; (3) sendThread.readResponse throws exception.
> 3. I am wondering what the purpose of pendingQueue is except for checking and if we can remove it. AFAICT that's the only concurrency concern.
>
> Hongchao Deng wrote:
> I come up with a better way if the pendingQueue is the only concern. It looks like pendingQueue doesn't care about ordering. So I can add packets to pendingQueue before writing to channel.
>
> fpj wrote:
> Check SendThread.readResponse() in ClientCnxn, pls.
>
> Hongchao Deng wrote:
> I did. I figure out it holds the assumptions:
> 1. pendingQueue only takes non-priming packets.
> 2. pendingQueue ordering is the same as packets sent in.
>
> I would re-clarify my point that I can remove the lock as long as the only interaction between read and write is the pendingQueue. :)
I've been revisiting the code and I can't think of any other potential problem.
- fpj
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review63604
-----------------------------------------------------------
On Dec. 3, 2014, 10:20 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2014, 10:20 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> 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
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by fp...@apache.org.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review63604
-----------------------------------------------------------
Looks better, Hongchao, thanks. I have a few more comments below, hope you don't mind.
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment105881>
Is the reason to have workSemaphore to block in doTransport when there isn't anything in the outgoing queue? If so, we might be better off with a blocking queue rather than a list as we have right now.
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment105880>
I'm not sure why you're not using the ChannelFuture returned here. See this connect() method in bookkeeper for an example https://github.com/apache/bookkeeper/blob/branch-4.0/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java#L112
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java
<https://reviews.apache.org/r/27244/#comment105876>
Could you elaborate on what you're trying to do with this NIOLock? I'm not sure what you mean with race cocnditions in the comment.
- fpj
On Nov. 24, 2014, 7:01 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Nov. 24, 2014, 7:01 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/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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Nov. 24, 2014, 7:01 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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/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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Nov. 22, 2014, 5:46 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Nov. 11, 2014, 8:54 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Nov. 10, 2014, 3:28 a.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Nov. 5, 2014, 6:03 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Nov. 5, 2014, 6:02 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Nov. 5, 2014, 5:28 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Nov. 5, 2014, 4:17 a.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Nov. 5, 2014, 2:31 a.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Nov. 4, 2014, 3:57 a.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Nov. 3, 2014, 10:12 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Nov. 3, 2014, 9:35 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Nov. 2, 2014, 8:27 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Nov. 2, 2014, 8:22 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Nov. 2, 2014, 3:28 a.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Nov. 1, 2014, 10:41 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by fp...@apache.org.
-----------------------------------------------------------
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>.
-----------------------------------------------------------
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 (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Oct. 31, 2014, 4:25 a.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Oct. 31, 2014, 2:13 a.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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/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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Oct. 31, 2014, 2:09 a.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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/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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/
-----------------------------------------------------------
(Updated Oct. 30, 2014, 10:53 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZOOKEEPER-2069
Diffs (updated)
-----
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/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