Hadoop QA
added a comment - 06/Mar/10 05:26 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12437783/ZOOKEEPER-622.patch
against trunk revision 919640.
+1 @author. The patch does not contain any @author tags.
-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no tests are needed for this patch.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/132/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/132/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/132/console
This message is automatically generated.

Hadoop QA
added a comment - 04/Mar/10 00:44 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12437783/ZOOKEEPER-622.patch
against trunk revision 918757.
+1 @author. The patch does not contain any @author tags.
-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no tests are needed for this patch.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/77/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/77/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/77/console
This message is automatically generated.

This patch works but is still in rough shape! The multi host test servers are started on separate ports and are running concurrently with the single host test server. Would be helpful to make this more clear.

Paths still hardcoded to /tmp.

zooX.cfg are in the patch but are generated by generateCfgs.sh

I encountered a ZCONNECTIONLOSS from an operation executed after the client reconnects to a new host. Adds an awkward section to the test case. I guess in some sense the client should not be oblivious to one of these "lucky" disconnect/reconnect situations.

Steven Cheng
added a comment - 23/Feb/10 08:32 This patch works but is still in rough shape! The multi host test servers are started on separate ports and are running concurrently with the single host test server. Would be helpful to make this more clear.
Paths still hardcoded to /tmp.
zooX.cfg are in the patch but are generated by generateCfgs.sh
I encountered a ZCONNECTIONLOSS from an operation executed after the client reconnects to a new host. Adds an awkward section to the test case. I guess in some sense the client should not be oblivious to one of these "lucky" disconnect/reconnect situations.

Steven Cheng
added a comment - 23/Feb/10 08:21 A working test case for watch transfer with multiple hosts. I left testScript in there because it is quite useful for basic sanity testing for the scripts.

Benjamin Reed
added a comment - 03/Feb/10 18:40 sorry steven, i didn't notice that you had commented. yes, please finish the test you can make simplifying assumptions such as /tmp and i can help you clean it up once things are working. thanx!

Hi Ben, sorry about that, this was just a rough cut patch, I got stuck when I tried to add zkMultiServer.sh into the tests since the Zookeeper server gets started in main() rather than in the test classes, so I posted it here to see if I was on the right track. Not sure if that was the right thing to do, mainly didn't want to go through and start changing all of the test setup without checking first.

If there isn't anything wrong with moving the Zookeeper server startup into the setup() methods of the test classes, I can probably get the patch starting up multiple servers in /tmp and the basic test in place after some work. To handle $base_dir I was planning to generate the config files somewhere in $base_dir and point the server to them.

Steven Cheng
added a comment - 28/Jan/10 02:07 Hi Ben, sorry about that, this was just a rough cut patch, I got stuck when I tried to add zkMultiServer.sh into the tests since the Zookeeper server gets started in main() rather than in the test classes, so I posted it here to see if I was on the right track. Not sure if that was the right thing to do, mainly didn't want to go through and start changing all of the test setup without checking first.
If there isn't anything wrong with moving the Zookeeper server startup into the setup() methods of the test classes, I can probably get the patch starting up multiple servers in /tmp and the basic test in place after some work. To handle $base_dir I was planning to generate the config files somewhere in $base_dir and point the server to them.

Benjamin Reed
added a comment - 27/Jan/10 14:55 sorry, i looked at this a bit more closely and i realized that we can't really assume that there is a /tmp. we should be using $
{base_dir}
/build/tmp/ i'll take a whack at fixing the zkMultiServer.sh.

good catch steven and excellent patch! will you put the license header in the generateCfgs.sh script? will you also put the header at the end of localserverlist (making it a footer to avoid a release warning?

Benjamin Reed
added a comment - 27/Jan/10 14:34 good catch steven and excellent patch! will you put the license header in the generateCfgs.sh script? will you also put the header at the end of localserverlist (making it a footer to avoid a release warning?

Steven Cheng
added a comment - 16/Jan/10 23:04 Started on adding multi server testing for C client, started creating the scripts, got stuck. main() in TestDriver.cc will always start the single server.
This patch has the scripts that I created so far.
It looks like adding this will need quite a few changes. Should I open a new JIRA?

this looks like a critical problem. This could cause setwatches never to be set if the count is initialized to 0 for all the three watches. We should definitely have a test case for this. How about setting a watch on a zookeeper server and shutting it down and making sure that the watch gets fired on the second server it reconnects to?

Mahadev konar
added a comment - 13/Dec/09 01:05 this looks like a critical problem. This could cause setwatches never to be set if the count is initialized to 0 for all the three watches. We should definitely have a test case for this. How about setting a watch on a zookeeper server and shutting it down and making sure that the watch gets fired on the second server it reconnects to?