zkServer.sh is looking for clientPort in config file, but it may no longer be there with ZK-1411

Details

Description

zkServer.sh is currently looking for "clientPort" entry in the static configuration file and uses it to contact the server.

With ZOOKEEPER-1411 clientPort is part of the dynamic configuration, and may appear in the separate dynamic configuration file. The "clientPort" entry may no longer be in the static config file.

With the proposed patch zkServer.sh first looks in the old (static) config file, then if clientPort is not there, it figures out the id of the server by looking at myid file, and then using that id finds the client port in the dynamic config file.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify 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 (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

Hadoop QA
added a comment - 22/Jan/13 06:30 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12565908/ZOOKEEPER-1625.patch
against trunk revision 1434978.
+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 new tests are needed for this patch.
Also please list what manual steps were performed to verify 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 (version 1.3.9) 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: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1352//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1352//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1352//console
This message is automatically generated.

Alexander Shraer
added a comment - 22/Jan/13 06:32 The patch includes only changes to zkServer.sh. I manually verified that it finds the client port in static or dynamic config file or reports an error if the port is not found.

Patrick Hunt
added a comment - 23/Jan/13 19:16 Previously we relied on the server code itself to check that the client port parameter was set, however now we're additionally checking in the zkServer script. Seems like an unnecessary check.
also is the following necessary/useful?
+ echo "Client port not found in static config file. Looking in dynamic config file."

with 1411 the client port may be correctly defined in the dynamic configuration file - it doesn't need to be in the old config file, the server will not detect that anything is wrong because everything is actually fine. The message you would get when running "zkServer.sh status" is "the server is probably not running", which is not helpful - the server is running but the script didn't find the clientport. This should be reported differently by the script. The message you quote is indeed not necessary, but may be useful to identify what really happened if something goes wrong.

Alexander Shraer
added a comment - 23/Jan/13 19:24 Hi Patrick,
with 1411 the client port may be correctly defined in the dynamic configuration file - it doesn't need to be in the old config file, the server will not detect that anything is wrong because everything is actually fine. The message you would get when running "zkServer.sh status" is "the server is probably not running", which is not helpful - the server is running but the script didn't find the clientport. This should be reported differently by the script. The message you quote is indeed not necessary, but may be useful to identify what really happened if something goes wrong.