Details

Description

This patch is mostly the same patch as my last one for ZOOKEEPER-823 minus everything Netty related. This means this patch only extract all NIO specific code in the class ClientCnxnSocketNIO which extends ClientCnxnSocket.
I've redone this patch from current trunk step by step now and couldn't find any logical error. I've already done a couple of successful test runs and will continue to do so this night.

It would be nice, if we could apply this patch as soon as possible to trunk. This allows us to continue to work on the netty integration without blocking the ClientCnxn class. Adding Netty after this patch should be only a matter of adding the ClientCnxnSocketNetty class with the appropriate test cases.

You could help me by reviewing the patch and by running it on whatever test server you have available. Please send me any complete failure log you should encounter to thomas at koch point ro. Thx!

In our build.xml we still have the old Sun link for javadoc ( http://java.sun.com/javase/6/docs/api/ ). This link redirects to http://download.oracle.com/javase/6/docs/api/
However when trying the old Sun link in my browser I first got a timeout while the second attempt worked.
So one possibility would be that I just had bad luck? Or is there any cache between the javadoc task and the Sun website? I've introduced a new class ( java.util.concurrent.CopyOnWriteArraySet ). Maybe this class has not yet been used elsewhere and therefor I need to hit the Sun website while other builds are served from some cache?

Anyhow. It would be best, if the build would not need to access the internet at all. I don't now javadoc good enough, but the ant task documentation gives me the impression that it should be possible to build the docs also offline: http://ant.apache.org/manual/Tasks/javadoc.html
( see documentation for the "link" element )

Thomas Koch
added a comment - 10/Nov/10 09:14 In our build.xml we still have the old Sun link for javadoc ( http://java.sun.com/javase/6/docs/api/ ). This link redirects to http://download.oracle.com/javase/6/docs/api/
However when trying the old Sun link in my browser I first got a timeout while the second attempt worked.
So one possibility would be that I just had bad luck? Or is there any cache between the javadoc task and the Sun website? I've introduced a new class ( java.util.concurrent.CopyOnWriteArraySet ). Maybe this class has not yet been used elsewhere and therefor I need to hit the Sun website while other builds are served from some cache?
Anyhow. It would be best, if the build would not need to access the internet at all. I don't now javadoc good enough, but the ant task documentation gives me the impression that it should be possible to build the docs also offline: http://ant.apache.org/manual/Tasks/javadoc.html
( see documentation for the "link" element )

Hi Thomas. Still shaky legs on getting the patch queue up and working again. Shouldn't keep us from getting this committed though.

re javadoc, this is not an issue for the other patches afaict, any idea why it's just showing up for this patch?

There are two sets of tests, java and the c client binding. Unfortunately hudson currently does not highlight c failures on the summary page, you need to checkout the console (usually raw) in the case where the tests fail (but not java test).

Patrick Hunt
added a comment - 10/Nov/10 08:16 Hi Thomas. Still shaky legs on getting the patch queue up and working again. Shouldn't keep us from getting this committed though.
re javadoc, this is not an issue for the other patches afaict, any idea why it's just showing up for this patch?
There are two sets of tests, java and the c client binding. Unfortunately hudson currently does not highlight c failures on the summary page, you need to checkout the console (usually raw) in the case where the tests fail (but not java test).
Looking at console I see:
[exec] [exec] ZooKeeper server process failed ZooKeeper server NOT startedRunning
I've notified Nigel about this to see is he has insight (saw it on a couple other jiras). So far he hasn't had a chance to look into it.

you're terribly annoying me! Looking at your web interface I can't find any failed unit test for this build. And the javadoc warning is due to the acquisition of SUN by Oracle (and the following unavailability of the old sun javadoc websites). - Don't blame me for that!

Thomas Koch
added a comment - 10/Nov/10 08:03 Dear Hudson,
you're terribly annoying me! Looking at your web interface I can't find any failed unit test for this build. And the javadoc warning is due to the acquisition of SUN by Oracle (and the following unavailability of the old sun javadoc websites). - Don't blame me for that!

Patrick Hunt
added a comment - 09/Nov/10 17:19 Hi Thomas, thanks! One more request, can you make this a single diff? Otw the hudson patch queue doesn't work properly.
http://wiki.apache.org/hadoop/ZooKeeper/HowToContribute
(you probably need to "svn add" the new file for "svn diff" to pick it up)
Then cancel/submit the jira again to trigger the workflow.
Thanks!

Thomas Koch
added a comment - 08/Nov/10 18:29 just FYI: This is the ClientCnxnSocketNetty class that should be added after this jira has been applied and should bring Netty support for the java client.

Thomas Koch
added a comment - 07/Nov/10 21:11 I couldn't wait for the tests to finish now, but it compiles and I did only renames, comments, visibility change and removal of dead code since the last patch.

I've added some javadoc and renamed socket to clientCnxnSocket everywhere. I'll upload the patch when the tests completed.

Moving ZooKeeper.state to ClientCnxn.state came in handy at some point to avoid unnecessary redirection and to clean the object dependency graph ( ZOOKEEPER-837 ). The state variable is only once accessed from ZooKeeper but several times from ClientCnxn so it makes a lot of sense to move it where it is needed.

SessionExpiredException can actually remain private, but EndOfStreamException is used in ClientCnxnSocketNIO.

Thomas Koch
added a comment - 07/Nov/10 21:06 I've added some javadoc and renamed socket to clientCnxnSocket everywhere. I'll upload the patch when the tests completed.
Moving ZooKeeper.state to ClientCnxn.state came in handy at some point to avoid unnecessary redirection and to clean the object dependency graph ( ZOOKEEPER-837 ). The state variable is only once accessed from ZooKeeper but several times from ClientCnxn so it makes a lot of sense to move it where it is needed.
SessionExpiredException can actually remain private, but EndOfStreamException is used in ClientCnxnSocketNIO.

Some questions after reviewing your diff (comments from Jacob and Michi):

In ClientCnxn.java:

Calling the member variable 'socket' is somewhat confusing. Maybe rename to 'clientSocket'?

I am not sure about the motivation for moving the state member variable out of class ZooKeeper into ClientConnection – can you explain?

Does the move of the state member have to be implemented at the same time as the NIO related changes? If not, perhaps split the JIRA?

Why are the inner classes SessionExpiredException and EndOfStreamException now package static? Was there a scoping issue with getting the wrong type (there are already identically named classes in KeeperException.java).

Please add a detailed comment on the purpose and implementation of onConnected()

Jacob Levy
added a comment - 03/Nov/10 01:13 Some questions after reviewing your diff (comments from Jacob and Michi):
In ClientCnxn.java:
Calling the member variable 'socket' is somewhat confusing. Maybe rename to 'clientSocket'?
I am not sure about the motivation for moving the state member variable out of class ZooKeeper into ClientConnection – can you explain?
Does the move of the state member have to be implemented at the same time as the NIO related changes? If not, perhaps split the JIRA?
Why are the inner classes SessionExpiredException and EndOfStreamException now package static? Was there a scoping issue with getting the wrong type (there are already identically named classes in KeeperException.java).
Please add a detailed comment on the purpose and implementation of onConnected()

the patch looks good. are you proposing that we commit it? or are you still working on it? i don't mind pushing off the javadoc for a bit if you think things might change. (although it would be nice to get that class more firmed up before we commit really...) we should get the property doc in before we commit since that will not change.

One other nit, if you are willing: calling the ClientCxnSocket "socket" and using "getSocket" is a bit confusing since ClientCnxnSocket does not extend socket. It's a bit more verbose, but more clear if you call the member and method "clientCxnSocket" and "getClientCnxnSocket".

Benjamin Reed
added a comment - 01/Nov/10 17:09 the patch looks good. are you proposing that we commit it? or are you still working on it? i don't mind pushing off the javadoc for a bit if you think things might change. (although it would be nice to get that class more firmed up before we commit really...) we should get the property doc in before we commit since that will not change.
One other nit, if you are willing: calling the ClientCxnSocket "socket" and using "getSocket" is a bit confusing since ClientCnxnSocket does not extend socket. It's a bit more verbose, but more clear if you call the member and method "clientCxnSocket" and "getClientCnxnSocket".

you mean, I should add a class javadoc for ClientCnxnSocket and a javadoc for the socket property in ClientCnxn.SendThread? You're right. However I did not yet come to an end with thinking about an elegant structure for the Classes ClientCnxn, SendThread and ClientCnxnSocket. I believe that the ClientCnxnSocket class won't remain for long as it is in this patch.
For example SendThread and ClientCnxn have a circular dependency which I really don't like. Also both classes work on the common properties incomingBuffer and outgoingBuffer which is suboptimal.
So I'd like to ask for forgiveness for sparse (or inexistent) documentation until we settle on a final design.

I also want to start to learn the server code now to see, whether it makes sense to generalize certain things.

Thomas Koch
added a comment - 22/Oct/10 20:34 Hi Benjamin,
you mean, I should add a class javadoc for ClientCnxnSocket and a javadoc for the socket property in ClientCnxn.SendThread? You're right. However I did not yet come to an end with thinking about an elegant structure for the Classes ClientCnxn, SendThread and ClientCnxnSocket. I believe that the ClientCnxnSocket class won't remain for long as it is in this patch.
For example SendThread and ClientCnxn have a circular dependency which I really don't like. Also both classes work on the common properties incomingBuffer and outgoingBuffer which is suboptimal.
So I'd like to ask for forgiveness for sparse (or inexistent) documentation until we settle on a final design.
I also want to start to learn the server code now to see, whether it makes sense to generalize certain things.

Thomas, I assigned the jira to you because you're doing most/all the work to get this done, not as a work token. I believe you should get the credit when this patch gets committed.

Typically we use assignment (esp when a patch gets committed) to credit the author - that's one of the criteria we monitor when deciding on new committers (number and quality of patches, testing, conformance to community style guidelinesl, etc...)

Patrick Hunt
added a comment - 22/Oct/10 20:08 Thomas, I assigned the jira to you because you're doing most/all the work to get this done, not as a work token. I believe you should get the credit when this patch gets committed.
Typically we use assignment (esp when a patch gets committed) to credit the author - that's one of the criteria we monitor when deciding on new committers (number and quality of patches, testing, conformance to community style guidelinesl, etc...)
Feel free to reassign this to yourself (please).

Benjamin Reed
added a comment - 22/Oct/10 17:47 this is looking really nice. i'm not done reviewing, but i did want to note that you need to add the zookeeper.clientCxnSocket property to the doc. You should also javadoc that variable.