Re: Review Request 27244: ZOOKEEPER-2069

[Hongchao Deng]

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

[Hongchao Deng]

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

[Hongchao Deng]

> On Dec. 11, 2014, 11:16 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 134
> > 
> >
> > 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://re

Re: Review Request 27244: ZOOKEEPER-2069

[Hongchao Deng]

> On Dec. 11, 2014, 11:16 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 134
> > 
> >
> > 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

[fpj]

> On Dec. 11, 2014, 11:16 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 134
> > 
> >
> > 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

[Hongchao Deng]

> On Dec. 11, 2014, 11:16 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 134
> > 
> >
> > 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

[fpj]

> On Dec. 11, 2014, 11:16 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 1472
> > 
> >
> > 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
> > 
> >
> > 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

[Hongchao Deng]

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

[Rakesh R]

> On Dec. 11, 2014, 7:37 p.m., Rakesh R wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 108
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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

[Hongchao Deng]

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

[Hongchao Deng]

> On Dec. 11, 2014, 11:16 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 1472
> > 
> >
> > 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

[fpj]

> On Dec. 11, 2014, 10:53 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 70
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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

[Hongchao Deng]

> On Dec. 11, 2014, 11:16 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 134
> > 
> >
> > 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
> > 
> >
> > 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

[fpj]

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


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


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


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

[Hongchao Deng]

> On Dec. 11, 2014, 10:53 p.m., fpj wrote:
> > build.xml, line 42
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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

[Hongchao Deng]

> On Dec. 11, 2014, 7:37 p.m., Rakesh R wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 99
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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

[fpj]

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


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


If you end up generating a new patch, please remove this tab/spaces.



src/java/main/org/apache/zookeeper/ClientCnxn.java


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


See previous comment.



src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java


Why does pendingQueue need to be made concurrent?



src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java


TODOs require a jira number.



src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java


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

[Rakesh R]

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


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


Can we make the visibiliy of the member variables to private.



src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java


Could you tell me when this situation can come? Is this added for safer 
side?



src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java


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


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


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


Make it private static class WakeupPacket



src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java


Add channel details in the log like,

LOG.info("channel {} is disconnected", ctx.getChannel());



src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java


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

[Hongchao Deng]

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

[Hongchao Deng]

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

[Hongchao Deng]

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

[Hongchao Deng]

> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 188
> > 
> >
> > 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

[fpj]

> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 188
> > 
> >
> > 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

[Hongchao Deng]

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

[Hongchao Deng]

> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 66
> > 
> >
> > 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
> > 
> >
> > 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

[Hongchao Deng]

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

[fpj]

> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 66
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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

[Hongchao Deng]

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

[Hongchao Deng]

> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 188
> > 
> >
> > 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

[Hongchao Deng]

> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 66
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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

[fpj]

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


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


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


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

[Hongchao Deng]

> On Nov. 24, 2014, 6:53 p.m., Raul Gutierrez Segales wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocket.java, line 179
> > 
> >
> > 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

[Hongchao Deng]

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

[Raul Gutierrez Segales]

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


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

[Hongchao Deng]

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

[Hongchao Deng]

> On Nov. 21, 2014, 7:46 p.m., Raul Gutierrez Segales wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocket.java, line 179
> > 
> >
> > 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

[Raul Gutierrez Segales]

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


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


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


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


nit: i'd drop this comment



src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java


nit, simpler message:

```
throw new IOException("Channel exists: duplicate connect?");
```



src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java


nit: code is straightforward, i'd drop the comment



src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java


nit: newline



src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java


nit: drop the comment



src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java


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

[Hongchao Deng]

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

[Hongchao Deng]

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

[Hongchao Deng]

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

[Hongchao Deng]

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

[Hongchao Deng]

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

[Hongchao Deng]

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

[Hongchao Deng]

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

[Hongchao Deng]

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

[Hongchao Deng]

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

[Hongchao Deng]

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

[Hongchao Deng]

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

[Hongchao Deng]

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

[Hongchao Deng]

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

[Hongchao Deng]

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

[Hongchao Deng]

> On Nov. 1, 2014, 12:14 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocket.java, line 154
> > 
> >
> > "... 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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

[fpj]

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


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


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


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


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

[Hongchao Deng]

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

[Hongchao Deng]

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

[Hongchao Deng]

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

[Hongchao Deng]

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

[Hongchao Deng]

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

Review Request 27244: ZOOKEEPER-2069

[Hongchao Deng]

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