You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Thomas Koch <th...@koch.ro> on 2011/09/05 17:30:14 UTC
Review Request: Exception in ZooKeeper.toString
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1714/
-----------------------------------------------------------
Review request for zookeeper.
Summary
-------
.
This addresses bug ZOOKEEPER-786.
https://issues.apache.org/jira/browse/ZOOKEEPER-786
Diffs
-----
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 626da04
Diff: https://reviews.apache.org/r/1714/diff
Testing
-------
Thanks,
Thomas
Re: Review Request: ZOOKEEPER-786 Exception in ZooKeeper.toString
Posted by Patrick Hunt <ph...@apache.org>.
> On 2011-09-27 13:59:22, Camille Fournier wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, line 195
> > <https://reviews.apache.org/r/1714/diff/1/?file=37950#file37950line195>
> >
> > Why was this removed? It is not germane to the patch at hand.
>
> Thomas Koch wrote:
> You're right, the removal had nothing to do with the issue, but it was the right thing to do, once I've been there. It may happen in rare cases, that connect returns true already there, but primeConnection() is called anyways later from doTransport(). Having to possible call origins only adds to uncertainty.
> Did you observe any problems with this?
>
> Camille Fournier wrote:
> Are you sure that the lines in doTransport() will be called and executed if the socket connects immediately? I don't personally know whether that is the case or not. You didn't write any tests to show that this would happen, and you shouldn't have made the change for purposes of removing uncertainty without explicitly calling out what you were doing. If you are quite sure that even in the case of an immediate connection, OP_CONNECT will be set and we will hit that in doTransport, I'm fine with this change. But I don't like having changes like this snuck in under the radar, because if this is not the case and it causes a bug, it will be a massive pain to find.
>
> Camille Fournier wrote:
> It seems to me, by the by, that if a socket is immediately connected it will NOT set the selection key for OP_CONNECT, because state will be set to ST_CONNECTED (see the source for SocketChannelImpl, specifically connect:
>
> http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/nio/ch/SocketChannelImpl.java#SocketChannelImpl.connect%28java.net.SocketAddress%29
>
> and translateReadyOps:
>
> http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/nio/ch/SocketChannelImpl.java#SocketChannelImpl.translateReadyOps%28int%2Cint%2Csun.nio.ch.SelectionKeyImpl%29
>
> So, I'm not sure that this is a safe change to make, and you should always, always, always, be careful when changing anything to do with our NIO code unless you are really sure of the consequences.
>
> Patrick Hunt wrote:
> Camille, you're right, I missed this change. Thanks.
>
> There is definitely a reason for this! The connect call can return immediately with a connected indication, in which case we need to run the primeConnection code, if you don't OP_CONNECT will never be indicated (because the connection already took place) and primeConnection will therefore never be called in doTransport.
>
> I believe we saw this with Solaris (there was a bug on this but I can't find it either on apache nor sourceforge).
>
>
>
> Camille Fournier wrote:
> Ted deserves the credit, he pointed it out as part of 1174. If you don't mind, I will pull this bit out of the change in trunk when I check in 1174 there.
It would be better to keep the changes distinct. Would you mind committing this snippet first, as a second patch for 786, then apply 1174? Or I/you can entirely roll back this change from trunk, you can commit 1174, and then Thomas can update this patch and reapply. Which would you prefer?
- Patrick
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1714/#review2090
-----------------------------------------------------------
On 2011-09-14 22:18:24, Thomas Koch wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1714/
> -----------------------------------------------------------
>
> (Updated 2011-09-14 22:18:24)
>
>
> Review request for zookeeper.
>
>
> Summary
> -------
>
> .
>
>
> This addresses bug ZOOKEEPER-786.
> https://issues.apache.org/jira/browse/ZOOKEEPER-786
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 626da04
>
> Diff: https://reviews.apache.org/r/1714/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Thomas
>
>
Re: Review Request: ZOOKEEPER-786 Exception in ZooKeeper.toString
Posted by Patrick Hunt <ph...@apache.org>.
> On 2011-09-27 13:59:22, Camille Fournier wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, line 195
> > <https://reviews.apache.org/r/1714/diff/1/?file=37950#file37950line195>
> >
> > Why was this removed? It is not germane to the patch at hand.
>
> Thomas Koch wrote:
> You're right, the removal had nothing to do with the issue, but it was the right thing to do, once I've been there. It may happen in rare cases, that connect returns true already there, but primeConnection() is called anyways later from doTransport(). Having to possible call origins only adds to uncertainty.
> Did you observe any problems with this?
>
> Camille Fournier wrote:
> Are you sure that the lines in doTransport() will be called and executed if the socket connects immediately? I don't personally know whether that is the case or not. You didn't write any tests to show that this would happen, and you shouldn't have made the change for purposes of removing uncertainty without explicitly calling out what you were doing. If you are quite sure that even in the case of an immediate connection, OP_CONNECT will be set and we will hit that in doTransport, I'm fine with this change. But I don't like having changes like this snuck in under the radar, because if this is not the case and it causes a bug, it will be a massive pain to find.
>
> Camille Fournier wrote:
> It seems to me, by the by, that if a socket is immediately connected it will NOT set the selection key for OP_CONNECT, because state will be set to ST_CONNECTED (see the source for SocketChannelImpl, specifically connect:
>
> http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/nio/ch/SocketChannelImpl.java#SocketChannelImpl.connect%28java.net.SocketAddress%29
>
> and translateReadyOps:
>
> http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/nio/ch/SocketChannelImpl.java#SocketChannelImpl.translateReadyOps%28int%2Cint%2Csun.nio.ch.SelectionKeyImpl%29
>
> So, I'm not sure that this is a safe change to make, and you should always, always, always, be careful when changing anything to do with our NIO code unless you are really sure of the consequences.
>
> Patrick Hunt wrote:
> Camille, you're right, I missed this change. Thanks.
>
> There is definitely a reason for this! The connect call can return immediately with a connected indication, in which case we need to run the primeConnection code, if you don't OP_CONNECT will never be indicated (because the connection already took place) and primeConnection will therefore never be called in doTransport.
>
> I believe we saw this with Solaris (there was a bug on this but I can't find it either on apache nor sourceforge).
>
>
>
> Camille Fournier wrote:
> Ted deserves the credit, he pointed it out as part of 1174. If you don't mind, I will pull this bit out of the change in trunk when I check in 1174 there.
>
> Patrick Hunt wrote:
> It would be better to keep the changes distinct. Would you mind committing this snippet first, as a second patch for 786, then apply 1174? Or I/you can entirely roll back this change from trunk, you can commit 1174, and then Thomas can update this patch and reapply. Which would you prefer?
>
> Camille Fournier wrote:
> I'll roll back this change as a second patch for 786, then apply 1174. This just went into trunk right?
In trunk I see this:
sockKey = sock.register(selector, SelectionKey.OP_CONNECT);
sock.connect(addr);
however in branch-3.4 I see this:
boolean immediateConnect = sock.connect(addr);
sockKey = sock.register(selector, SelectionKey.OP_CONNECT);
if (immediateConnect) {
sendThread.primeConnection();
}
so yes, this patch was applied only to trunk, however, isn't this a bug? sock.register is being called after sock.connect.
Also, why was this patch applied to branch 3.4, but it's not available on trunk?
- Patrick
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1714/#review2090
-----------------------------------------------------------
On 2011-09-14 22:18:24, Thomas Koch wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1714/
> -----------------------------------------------------------
>
> (Updated 2011-09-14 22:18:24)
>
>
> Review request for zookeeper.
>
>
> Summary
> -------
>
> .
>
>
> This addresses bug ZOOKEEPER-786.
> https://issues.apache.org/jira/browse/ZOOKEEPER-786
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 626da04
>
> Diff: https://reviews.apache.org/r/1714/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Thomas
>
>
Re: Review Request: ZOOKEEPER-786 Exception in ZooKeeper.toString
Posted by Camille Fournier <sk...@gmail.com>.
> On 2011-09-27 13:59:22, Camille Fournier wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, line 195
> > <https://reviews.apache.org/r/1714/diff/1/?file=37950#file37950line195>
> >
> > Why was this removed? It is not germane to the patch at hand.
>
> Thomas Koch wrote:
> You're right, the removal had nothing to do with the issue, but it was the right thing to do, once I've been there. It may happen in rare cases, that connect returns true already there, but primeConnection() is called anyways later from doTransport(). Having to possible call origins only adds to uncertainty.
> Did you observe any problems with this?
>
> Camille Fournier wrote:
> Are you sure that the lines in doTransport() will be called and executed if the socket connects immediately? I don't personally know whether that is the case or not. You didn't write any tests to show that this would happen, and you shouldn't have made the change for purposes of removing uncertainty without explicitly calling out what you were doing. If you are quite sure that even in the case of an immediate connection, OP_CONNECT will be set and we will hit that in doTransport, I'm fine with this change. But I don't like having changes like this snuck in under the radar, because if this is not the case and it causes a bug, it will be a massive pain to find.
It seems to me, by the by, that if a socket is immediately connected it will NOT set the selection key for OP_CONNECT, because state will be set to ST_CONNECTED (see the source for SocketChannelImpl, specifically connect:
http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/nio/ch/SocketChannelImpl.java#SocketChannelImpl.connect%28java.net.SocketAddress%29
and translateReadyOps:
http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/nio/ch/SocketChannelImpl.java#SocketChannelImpl.translateReadyOps%28int%2Cint%2Csun.nio.ch.SelectionKeyImpl%29
So, I'm not sure that this is a safe change to make, and you should always, always, always, be careful when changing anything to do with our NIO code unless you are really sure of the consequences.
- Camille
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1714/#review2090
-----------------------------------------------------------
On 2011-09-14 22:18:24, Thomas Koch wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1714/
> -----------------------------------------------------------
>
> (Updated 2011-09-14 22:18:24)
>
>
> Review request for zookeeper.
>
>
> Summary
> -------
>
> .
>
>
> This addresses bug ZOOKEEPER-786.
> https://issues.apache.org/jira/browse/ZOOKEEPER-786
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 626da04
>
> Diff: https://reviews.apache.org/r/1714/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Thomas
>
>
Re: Review Request: ZOOKEEPER-786 Exception in ZooKeeper.toString
Posted by Thomas Koch <th...@koch.ro>.
> On 2011-09-27 13:59:22, Camille Fournier wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, line 195
> > <https://reviews.apache.org/r/1714/diff/1/?file=37950#file37950line195>
> >
> > Why was this removed? It is not germane to the patch at hand.
You're right, the removal had nothing to do with the issue, but it was the right thing to do, once I've been there. It may happen in rare cases, that connect returns true already there, but primeConnection() is called anyways later from doTransport(). Having to possible call origins only adds to uncertainty.
Did you observe any problems with this?
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1714/#review2090
-----------------------------------------------------------
On 2011-09-14 22:18:24, Thomas Koch wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1714/
> -----------------------------------------------------------
>
> (Updated 2011-09-14 22:18:24)
>
>
> Review request for zookeeper.
>
>
> Summary
> -------
>
> .
>
>
> This addresses bug ZOOKEEPER-786.
> https://issues.apache.org/jira/browse/ZOOKEEPER-786
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 626da04
>
> Diff: https://reviews.apache.org/r/1714/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Thomas
>
>
Re: Review Request: ZOOKEEPER-786 Exception in ZooKeeper.toString
Posted by Patrick Hunt <ph...@apache.org>.
> On 2011-09-27 13:59:22, Camille Fournier wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, line 195
> > <https://reviews.apache.org/r/1714/diff/1/?file=37950#file37950line195>
> >
> > Why was this removed? It is not germane to the patch at hand.
>
> Thomas Koch wrote:
> You're right, the removal had nothing to do with the issue, but it was the right thing to do, once I've been there. It may happen in rare cases, that connect returns true already there, but primeConnection() is called anyways later from doTransport(). Having to possible call origins only adds to uncertainty.
> Did you observe any problems with this?
>
> Camille Fournier wrote:
> Are you sure that the lines in doTransport() will be called and executed if the socket connects immediately? I don't personally know whether that is the case or not. You didn't write any tests to show that this would happen, and you shouldn't have made the change for purposes of removing uncertainty without explicitly calling out what you were doing. If you are quite sure that even in the case of an immediate connection, OP_CONNECT will be set and we will hit that in doTransport, I'm fine with this change. But I don't like having changes like this snuck in under the radar, because if this is not the case and it causes a bug, it will be a massive pain to find.
>
> Camille Fournier wrote:
> It seems to me, by the by, that if a socket is immediately connected it will NOT set the selection key for OP_CONNECT, because state will be set to ST_CONNECTED (see the source for SocketChannelImpl, specifically connect:
>
> http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/nio/ch/SocketChannelImpl.java#SocketChannelImpl.connect%28java.net.SocketAddress%29
>
> and translateReadyOps:
>
> http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/nio/ch/SocketChannelImpl.java#SocketChannelImpl.translateReadyOps%28int%2Cint%2Csun.nio.ch.SelectionKeyImpl%29
>
> So, I'm not sure that this is a safe change to make, and you should always, always, always, be careful when changing anything to do with our NIO code unless you are really sure of the consequences.
>
> Patrick Hunt wrote:
> Camille, you're right, I missed this change. Thanks.
>
> There is definitely a reason for this! The connect call can return immediately with a connected indication, in which case we need to run the primeConnection code, if you don't OP_CONNECT will never be indicated (because the connection already took place) and primeConnection will therefore never be called in doTransport.
>
> I believe we saw this with Solaris (there was a bug on this but I can't find it either on apache nor sourceforge).
>
>
>
> Camille Fournier wrote:
> Ted deserves the credit, he pointed it out as part of 1174. If you don't mind, I will pull this bit out of the change in trunk when I check in 1174 there.
>
> Patrick Hunt wrote:
> It would be better to keep the changes distinct. Would you mind committing this snippet first, as a second patch for 786, then apply 1174? Or I/you can entirely roll back this change from trunk, you can commit 1174, and then Thomas can update this patch and reapply. Which would you prefer?
>
> Camille Fournier wrote:
> I'll roll back this change as a second patch for 786, then apply 1174. This just went into trunk right?
>
> Patrick Hunt wrote:
> In trunk I see this:
>
> sockKey = sock.register(selector, SelectionKey.OP_CONNECT);
> sock.connect(addr);
>
> however in branch-3.4 I see this:
>
> boolean immediateConnect = sock.connect(addr);
> sockKey = sock.register(selector, SelectionKey.OP_CONNECT);
> if (immediateConnect) {
> sendThread.primeConnection();
> }
>
> so yes, this patch was applied only to trunk, however, isn't this a bug? sock.register is being called after sock.connect.
>
> Also, why was this patch applied to branch 3.4, but it's not available on trunk?
>
I see. 1174 was committed to branch-3.4 but this change caused a conflict for trunk.
Isn't this a timing issue then? registering after connecting?
- Patrick
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1714/#review2090
-----------------------------------------------------------
On 2011-09-14 22:18:24, Thomas Koch wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1714/
> -----------------------------------------------------------
>
> (Updated 2011-09-14 22:18:24)
>
>
> Review request for zookeeper.
>
>
> Summary
> -------
>
> .
>
>
> This addresses bug ZOOKEEPER-786.
> https://issues.apache.org/jira/browse/ZOOKEEPER-786
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 626da04
>
> Diff: https://reviews.apache.org/r/1714/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Thomas
>
>
Re: Review Request: ZOOKEEPER-786 Exception in ZooKeeper.toString
Posted by Camille Fournier <sk...@gmail.com>.
> On 2011-09-27 13:59:22, Camille Fournier wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, line 195
> > <https://reviews.apache.org/r/1714/diff/1/?file=37950#file37950line195>
> >
> > Why was this removed? It is not germane to the patch at hand.
>
> Thomas Koch wrote:
> You're right, the removal had nothing to do with the issue, but it was the right thing to do, once I've been there. It may happen in rare cases, that connect returns true already there, but primeConnection() is called anyways later from doTransport(). Having to possible call origins only adds to uncertainty.
> Did you observe any problems with this?
>
> Camille Fournier wrote:
> Are you sure that the lines in doTransport() will be called and executed if the socket connects immediately? I don't personally know whether that is the case or not. You didn't write any tests to show that this would happen, and you shouldn't have made the change for purposes of removing uncertainty without explicitly calling out what you were doing. If you are quite sure that even in the case of an immediate connection, OP_CONNECT will be set and we will hit that in doTransport, I'm fine with this change. But I don't like having changes like this snuck in under the radar, because if this is not the case and it causes a bug, it will be a massive pain to find.
>
> Camille Fournier wrote:
> It seems to me, by the by, that if a socket is immediately connected it will NOT set the selection key for OP_CONNECT, because state will be set to ST_CONNECTED (see the source for SocketChannelImpl, specifically connect:
>
> http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/nio/ch/SocketChannelImpl.java#SocketChannelImpl.connect%28java.net.SocketAddress%29
>
> and translateReadyOps:
>
> http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/nio/ch/SocketChannelImpl.java#SocketChannelImpl.translateReadyOps%28int%2Cint%2Csun.nio.ch.SelectionKeyImpl%29
>
> So, I'm not sure that this is a safe change to make, and you should always, always, always, be careful when changing anything to do with our NIO code unless you are really sure of the consequences.
>
> Patrick Hunt wrote:
> Camille, you're right, I missed this change. Thanks.
>
> There is definitely a reason for this! The connect call can return immediately with a connected indication, in which case we need to run the primeConnection code, if you don't OP_CONNECT will never be indicated (because the connection already took place) and primeConnection will therefore never be called in doTransport.
>
> I believe we saw this with Solaris (there was a bug on this but I can't find it either on apache nor sourceforge).
>
>
>
> Camille Fournier wrote:
> Ted deserves the credit, he pointed it out as part of 1174. If you don't mind, I will pull this bit out of the change in trunk when I check in 1174 there.
>
> Patrick Hunt wrote:
> It would be better to keep the changes distinct. Would you mind committing this snippet first, as a second patch for 786, then apply 1174? Or I/you can entirely roll back this change from trunk, you can commit 1174, and then Thomas can update this patch and reapply. Which would you prefer?
I'll roll back this change as a second patch for 786, then apply 1174. This just went into trunk right?
- Camille
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1714/#review2090
-----------------------------------------------------------
On 2011-09-14 22:18:24, Thomas Koch wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1714/
> -----------------------------------------------------------
>
> (Updated 2011-09-14 22:18:24)
>
>
> Review request for zookeeper.
>
>
> Summary
> -------
>
> .
>
>
> This addresses bug ZOOKEEPER-786.
> https://issues.apache.org/jira/browse/ZOOKEEPER-786
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 626da04
>
> Diff: https://reviews.apache.org/r/1714/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Thomas
>
>
Re: Review Request: ZOOKEEPER-786 Exception in ZooKeeper.toString
Posted by Camille Fournier <sk...@gmail.com>.
> On 2011-09-27 13:59:22, Camille Fournier wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, line 195
> > <https://reviews.apache.org/r/1714/diff/1/?file=37950#file37950line195>
> >
> > Why was this removed? It is not germane to the patch at hand.
>
> Thomas Koch wrote:
> You're right, the removal had nothing to do with the issue, but it was the right thing to do, once I've been there. It may happen in rare cases, that connect returns true already there, but primeConnection() is called anyways later from doTransport(). Having to possible call origins only adds to uncertainty.
> Did you observe any problems with this?
Are you sure that the lines in doTransport() will be called and executed if the socket connects immediately? I don't personally know whether that is the case or not. You didn't write any tests to show that this would happen, and you shouldn't have made the change for purposes of removing uncertainty without explicitly calling out what you were doing. If you are quite sure that even in the case of an immediate connection, OP_CONNECT will be set and we will hit that in doTransport, I'm fine with this change. But I don't like having changes like this snuck in under the radar, because if this is not the case and it causes a bug, it will be a massive pain to find.
- Camille
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1714/#review2090
-----------------------------------------------------------
On 2011-09-14 22:18:24, Thomas Koch wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1714/
> -----------------------------------------------------------
>
> (Updated 2011-09-14 22:18:24)
>
>
> Review request for zookeeper.
>
>
> Summary
> -------
>
> .
>
>
> This addresses bug ZOOKEEPER-786.
> https://issues.apache.org/jira/browse/ZOOKEEPER-786
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 626da04
>
> Diff: https://reviews.apache.org/r/1714/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Thomas
>
>
Re: Review Request: ZOOKEEPER-786 Exception in ZooKeeper.toString
Posted by Camille Fournier <sk...@gmail.com>.
> On 2011-09-27 13:59:22, Camille Fournier wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, line 195
> > <https://reviews.apache.org/r/1714/diff/1/?file=37950#file37950line195>
> >
> > Why was this removed? It is not germane to the patch at hand.
>
> Thomas Koch wrote:
> You're right, the removal had nothing to do with the issue, but it was the right thing to do, once I've been there. It may happen in rare cases, that connect returns true already there, but primeConnection() is called anyways later from doTransport(). Having to possible call origins only adds to uncertainty.
> Did you observe any problems with this?
>
> Camille Fournier wrote:
> Are you sure that the lines in doTransport() will be called and executed if the socket connects immediately? I don't personally know whether that is the case or not. You didn't write any tests to show that this would happen, and you shouldn't have made the change for purposes of removing uncertainty without explicitly calling out what you were doing. If you are quite sure that even in the case of an immediate connection, OP_CONNECT will be set and we will hit that in doTransport, I'm fine with this change. But I don't like having changes like this snuck in under the radar, because if this is not the case and it causes a bug, it will be a massive pain to find.
>
> Camille Fournier wrote:
> It seems to me, by the by, that if a socket is immediately connected it will NOT set the selection key for OP_CONNECT, because state will be set to ST_CONNECTED (see the source for SocketChannelImpl, specifically connect:
>
> http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/nio/ch/SocketChannelImpl.java#SocketChannelImpl.connect%28java.net.SocketAddress%29
>
> and translateReadyOps:
>
> http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/nio/ch/SocketChannelImpl.java#SocketChannelImpl.translateReadyOps%28int%2Cint%2Csun.nio.ch.SelectionKeyImpl%29
>
> So, I'm not sure that this is a safe change to make, and you should always, always, always, be careful when changing anything to do with our NIO code unless you are really sure of the consequences.
>
> Patrick Hunt wrote:
> Camille, you're right, I missed this change. Thanks.
>
> There is definitely a reason for this! The connect call can return immediately with a connected indication, in which case we need to run the primeConnection code, if you don't OP_CONNECT will never be indicated (because the connection already took place) and primeConnection will therefore never be called in doTransport.
>
> I believe we saw this with Solaris (there was a bug on this but I can't find it either on apache nor sourceforge).
>
>
Ted deserves the credit, he pointed it out as part of 1174. If you don't mind, I will pull this bit out of the change in trunk when I check in 1174 there.
- Camille
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1714/#review2090
-----------------------------------------------------------
On 2011-09-14 22:18:24, Thomas Koch wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1714/
> -----------------------------------------------------------
>
> (Updated 2011-09-14 22:18:24)
>
>
> Review request for zookeeper.
>
>
> Summary
> -------
>
> .
>
>
> This addresses bug ZOOKEEPER-786.
> https://issues.apache.org/jira/browse/ZOOKEEPER-786
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 626da04
>
> Diff: https://reviews.apache.org/r/1714/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Thomas
>
>
Re: Review Request: ZOOKEEPER-786 Exception in ZooKeeper.toString
Posted by Patrick Hunt <ph...@apache.org>.
> On 2011-09-27 13:59:22, Camille Fournier wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, line 195
> > <https://reviews.apache.org/r/1714/diff/1/?file=37950#file37950line195>
> >
> > Why was this removed? It is not germane to the patch at hand.
>
> Thomas Koch wrote:
> You're right, the removal had nothing to do with the issue, but it was the right thing to do, once I've been there. It may happen in rare cases, that connect returns true already there, but primeConnection() is called anyways later from doTransport(). Having to possible call origins only adds to uncertainty.
> Did you observe any problems with this?
>
> Camille Fournier wrote:
> Are you sure that the lines in doTransport() will be called and executed if the socket connects immediately? I don't personally know whether that is the case or not. You didn't write any tests to show that this would happen, and you shouldn't have made the change for purposes of removing uncertainty without explicitly calling out what you were doing. If you are quite sure that even in the case of an immediate connection, OP_CONNECT will be set and we will hit that in doTransport, I'm fine with this change. But I don't like having changes like this snuck in under the radar, because if this is not the case and it causes a bug, it will be a massive pain to find.
>
> Camille Fournier wrote:
> It seems to me, by the by, that if a socket is immediately connected it will NOT set the selection key for OP_CONNECT, because state will be set to ST_CONNECTED (see the source for SocketChannelImpl, specifically connect:
>
> http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/nio/ch/SocketChannelImpl.java#SocketChannelImpl.connect%28java.net.SocketAddress%29
>
> and translateReadyOps:
>
> http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/nio/ch/SocketChannelImpl.java#SocketChannelImpl.translateReadyOps%28int%2Cint%2Csun.nio.ch.SelectionKeyImpl%29
>
> So, I'm not sure that this is a safe change to make, and you should always, always, always, be careful when changing anything to do with our NIO code unless you are really sure of the consequences.
Camille, you're right, I missed this change. Thanks.
There is definitely a reason for this! The connect call can return immediately with a connected indication, in which case we need to run the primeConnection code, if you don't OP_CONNECT will never be indicated (because the connection already took place) and primeConnection will therefore never be called in doTransport.
I believe we saw this with Solaris (there was a bug on this but I can't find it either on apache nor sourceforge).
- Patrick
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1714/#review2090
-----------------------------------------------------------
On 2011-09-14 22:18:24, Thomas Koch wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1714/
> -----------------------------------------------------------
>
> (Updated 2011-09-14 22:18:24)
>
>
> Review request for zookeeper.
>
>
> Summary
> -------
>
> .
>
>
> This addresses bug ZOOKEEPER-786.
> https://issues.apache.org/jira/browse/ZOOKEEPER-786
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 626da04
>
> Diff: https://reviews.apache.org/r/1714/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Thomas
>
>
Re: Review Request: ZOOKEEPER-786 Exception in ZooKeeper.toString
Posted by Camille Fournier <sk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1714/#review2090
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java
<https://reviews.apache.org/r/1714/#comment4735>
Why was this removed? It is not germane to the patch at hand.
- Camille
On 2011-09-14 22:18:24, Thomas Koch wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1714/
> -----------------------------------------------------------
>
> (Updated 2011-09-14 22:18:24)
>
>
> Review request for zookeeper.
>
>
> Summary
> -------
>
> .
>
>
> This addresses bug ZOOKEEPER-786.
> https://issues.apache.org/jira/browse/ZOOKEEPER-786
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 626da04
>
> Diff: https://reviews.apache.org/r/1714/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Thomas
>
>
Re: Review Request: ZOOKEEPER-786 Exception in ZooKeeper.toString
Posted by Patrick Hunt <ph...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1714/
-----------------------------------------------------------
(Updated 2011-09-14 22:18:24.072279)
Review request for zookeeper.
Summary (updated)
-------
.
This addresses bug ZOOKEEPER-786.
https://issues.apache.org/jira/browse/ZOOKEEPER-786
Diffs
-----
src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 626da04
Diff: https://reviews.apache.org/r/1714/diff
Testing
-------
Thanks,
Thomas