You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by fp...@apache.org on 2014/12/03 00:43:42 UTC
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
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 66
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line66>
> >
> > Is the reason to have workSemaphore to block in doTransport when there isn't anything in the outgoing queue? If so, we might be better off with a blocking queue rather than a list as we have right now.
Sounds good on my side. I tried to make minimal changes. Do you think I should make it in here or create another JIRA?
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 107
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line107>
> >
> > I'm not sure why you're not using the ChannelFuture returned here. See this connect() method in bookkeeper for an example https://github.com/apache/bookkeeper/blob/branch-4.0/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java#L112
Good. Fix it next
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 188
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line188>
> >
> > Could you elaborate on what you're trying to do with this NIOLock? I'm not sure what you mean with race cocnditions in the comment.
1. The intuition is that in NIO client all reads and writes are sequential.
2. One problem I found: (1) packet is removed from outgoingQueue; (2) before the packet is added to pendingQueue; (3) sendThread.readResponse throws exception.
3. I am wondering what the purpose of pendingQueue is except for checking and if we can remove it. AFAICT that's the only concurrency concern.
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review63604
-----------------------------------------------------------
On Nov. 24, 2014, 7:01 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Nov. 24, 2014, 7:01 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07
> src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 5ca0ba7
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java adb27ee
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteBase.java PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteHammerTest.java PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/27244/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 188
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line188>
> >
> > Could you elaborate on what you're trying to do with this NIOLock? I'm not sure what you mean with race cocnditions in the comment.
>
> Hongchao Deng wrote:
> 1. The intuition is that in NIO client all reads and writes are sequential.
> 2. One problem I found: (1) packet is removed from outgoingQueue; (2) before the packet is added to pendingQueue; (3) sendThread.readResponse throws exception.
> 3. I am wondering what the purpose of pendingQueue is except for checking and if we can remove it. AFAICT that's the only concurrency concern.
I come up with a better way if the pendingQueue is the only concern. It looks like pendingQueue doesn't care about ordering. So I can add packets to pendingQueue before writing to channel.
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review63604
-----------------------------------------------------------
On Nov. 24, 2014, 7:01 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Nov. 24, 2014, 7:01 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07
> src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 5ca0ba7
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java adb27ee
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteBase.java PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteHammerTest.java PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/27244/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by fp...@apache.org.
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 66
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line66>
> >
> > Is the reason to have workSemaphore to block in doTransport when there isn't anything in the outgoing queue? If so, we might be better off with a blocking queue rather than a list as we have right now.
>
> Hongchao Deng wrote:
> Sounds good on my side. I tried to make minimal changes. Do you think I should make it in here or create another JIRA?
I'd rather have it fixed here, but depends on how much more energy you have to put into this patch.
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 107
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line107>
> >
> > I'm not sure why you're not using the ChannelFuture returned here. See this connect() method in bookkeeper for an example https://github.com/apache/bookkeeper/blob/branch-4.0/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java#L112
>
> Hongchao Deng wrote:
> Good. Fix it next
Ok.
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 188
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line188>
> >
> > Could you elaborate on what you're trying to do with this NIOLock? I'm not sure what you mean with race cocnditions in the comment.
>
> Hongchao Deng wrote:
> 1. The intuition is that in NIO client all reads and writes are sequential.
> 2. One problem I found: (1) packet is removed from outgoingQueue; (2) before the packet is added to pendingQueue; (3) sendThread.readResponse throws exception.
> 3. I am wondering what the purpose of pendingQueue is except for checking and if we can remove it. AFAICT that's the only concurrency concern.
>
> Hongchao Deng wrote:
> I come up with a better way if the pendingQueue is the only concern. It looks like pendingQueue doesn't care about ordering. So I can add packets to pendingQueue before writing to channel.
Check SendThread.readResponse() in ClientCnxn, pls.
- fpj
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review63604
-----------------------------------------------------------
On Dec. 3, 2014, 1:52 a.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2014, 1:52 a.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07
> src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 5ca0ba7
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java adb27ee
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteBase.java PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteHammerTest.java PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/27244/diff/
>
>
> Testing
> -------
>
> 1. use ChannelFuture returned by connect()
> 2. remove NIOLock
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 66
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line66>
> >
> > Is the reason to have workSemaphore to block in doTransport when there isn't anything in the outgoing queue? If so, we might be better off with a blocking queue rather than a list as we have right now.
>
> Hongchao Deng wrote:
> Sounds good on my side. I tried to make minimal changes. Do you think I should make it in here or create another JIRA?
>
> fpj wrote:
> I'd rather have it fixed here, but depends on how much more energy you have to put into this patch.
I get a new patch written. Tested in local jenkins.
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 188
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line188>
> >
> > Could you elaborate on what you're trying to do with this NIOLock? I'm not sure what you mean with race cocnditions in the comment.
>
> Hongchao Deng wrote:
> 1. The intuition is that in NIO client all reads and writes are sequential.
> 2. One problem I found: (1) packet is removed from outgoingQueue; (2) before the packet is added to pendingQueue; (3) sendThread.readResponse throws exception.
> 3. I am wondering what the purpose of pendingQueue is except for checking and if we can remove it. AFAICT that's the only concurrency concern.
>
> Hongchao Deng wrote:
> I come up with a better way if the pendingQueue is the only concern. It looks like pendingQueue doesn't care about ordering. So I can add packets to pendingQueue before writing to channel.
>
> fpj wrote:
> Check SendThread.readResponse() in ClientCnxn, pls.
I did. I figure out it holds the assumptions:
1. pendingQueue only takes non-priming packets.
2. pendingQueue ordering is the same as packets sent in.
I would re-clarify my point that I can remove the lock as long as the only interaction between read and write is the pendingQueue. :)
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review63604
-----------------------------------------------------------
On Dec. 3, 2014, 9:50 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2014, 9:50 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07
> src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 5ca0ba7
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java adb27ee
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION
> src/java/main/org/apache/zookeeper/ZooKeeper.java dd13cc9
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteBase.java PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteHammerTest.java PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/27244/diff/
>
>
> Testing
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 188
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line188>
> >
> > Could you elaborate on what you're trying to do with this NIOLock? I'm not sure what you mean with race cocnditions in the comment.
>
> Hongchao Deng wrote:
> 1. The intuition is that in NIO client all reads and writes are sequential.
> 2. One problem I found: (1) packet is removed from outgoingQueue; (2) before the packet is added to pendingQueue; (3) sendThread.readResponse throws exception.
> 3. I am wondering what the purpose of pendingQueue is except for checking and if we can remove it. AFAICT that's the only concurrency concern.
>
> Hongchao Deng wrote:
> I come up with a better way if the pendingQueue is the only concern. It looks like pendingQueue doesn't care about ordering. So I can add packets to pendingQueue before writing to channel.
>
> fpj wrote:
> Check SendThread.readResponse() in ClientCnxn, pls.
>
> Hongchao Deng wrote:
> I did. I figure out it holds the assumptions:
> 1. pendingQueue only takes non-priming packets.
> 2. pendingQueue ordering is the same as packets sent in.
>
> I would re-clarify my point that I can remove the lock as long as the only interaction between read and write is the pendingQueue. :)
>
> fpj wrote:
> I've been revisiting the code and I can't think of any other potential problem.
Cool.
I think my new patch is making too many changes. I will change the last thing you pointed out and update them only. Thanks!
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review63604
-----------------------------------------------------------
On Dec. 3, 2014, 10:20 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2014, 10:20 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07
> src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 5ca0ba7
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java adb27ee
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION
> src/java/main/org/apache/zookeeper/ZooKeeper.java dd13cc9
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteBase.java PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteHammerTest.java PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/27244/diff/
>
>
> Testing
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
Posted by fp...@apache.org.
> On Dec. 2, 2014, 11:43 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 188
> > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line188>
> >
> > Could you elaborate on what you're trying to do with this NIOLock? I'm not sure what you mean with race cocnditions in the comment.
>
> Hongchao Deng wrote:
> 1. The intuition is that in NIO client all reads and writes are sequential.
> 2. One problem I found: (1) packet is removed from outgoingQueue; (2) before the packet is added to pendingQueue; (3) sendThread.readResponse throws exception.
> 3. I am wondering what the purpose of pendingQueue is except for checking and if we can remove it. AFAICT that's the only concurrency concern.
>
> Hongchao Deng wrote:
> I come up with a better way if the pendingQueue is the only concern. It looks like pendingQueue doesn't care about ordering. So I can add packets to pendingQueue before writing to channel.
>
> fpj wrote:
> Check SendThread.readResponse() in ClientCnxn, pls.
>
> Hongchao Deng wrote:
> I did. I figure out it holds the assumptions:
> 1. pendingQueue only takes non-priming packets.
> 2. pendingQueue ordering is the same as packets sent in.
>
> I would re-clarify my point that I can remove the lock as long as the only interaction between read and write is the pendingQueue. :)
I've been revisiting the code and I can't think of any other potential problem.
- fpj
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review63604
-----------------------------------------------------------
On Dec. 3, 2014, 10:20 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2014, 10:20 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZOOKEEPER-2069
>
>
> Diffs
> -----
>
> build.xml bb5ff4f
> src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07
> src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 5ca0ba7
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java adb27ee
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION
> src/java/main/org/apache/zookeeper/ZooKeeper.java dd13cc9
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteBase.java PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteHammerTest.java PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/27244/diff/
>
>
> Testing
> -------
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>