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