Made the timeout parameter configurable and set-able.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12563296/HADOOP-9106v1.patch
against trunk revision .

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+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 failed these unit tests in hadoop-common-project/hadoop-common:

org.apache.hadoop.ipc.TestIPC

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1944//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1944//console

This message is automatically generated.

Need to correct the unit test

Quick comments:

1. Client.java - please make the newly added method setConnectionTimeout into static
2. The getConnectionTimeout() method is not needed. Please save the connection timeout in a final member varilable in Client instead of getting it from the configuration every time.

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12563371/HADOOP-9106v2.patch
against trunk revision .

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+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 unit tests in hadoop-common-project/hadoop-common.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1945//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1945//console

This message is automatically generated.

Suresh,
I think making setConnectionTimeout static will not allow configuring 'per-client' which is desired.
Concur with replacing the getConnectionTimeout.

Maybe we can change the default 20s to a smaller value as well, i think 20s is too large, let's tweak it to 1s or some other values ?

think making setConnectionTimeout static will not allow configuring 'per-client' which is desired

When a Client is created conf is passed to it in constructor. Given that the connection timeout in this patch is reading the timeout off of this conf, the timeout should be set in the conf object before it is used in the Client constructor and it can be done using a static method.

Maybe we can change the default 20s to a smaller value as well,

I think 20s should be fine. With this configurability you can change it. Lets do it in another jira.

Made the set and get static and did not use a member variable to match the same pattern in Client for setPingInterval and getPingInterval functions. Added the default to the core-default.xml

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12563763/HADOOP-9106v3.patch
against trunk revision .

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+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 failed these unit tests in hadoop-common-project/hadoop-common:

org.apache.hadoop.ha.TestZKFailoverController

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2013//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2013//console

This message is automatically generated.

Robert, can you please address my previous comment, either with code change or why that change is not necessary.

Please save the connection timeout in a final member varilable in Client instead of getting it from the configuration every time.

Other comments:

1. final modifier for static methods seems unnecessary. I know there are arguments for using it. But not sure if it is required in this case.
2. Change "final public static" to "public static final"

BTW changing "IPC_CLIENT_CONNECTION_TIMEOUT_KEY" t0 "IPC_CLIENT_CONNECT_TIMEOUT_KEY" is a good change. I should have spotted it.

Suresh, using a class member variable would require it to be set only once in the constructor unless we make the setter function non-static. The passing of the conf in the setter makes the use of static desirable but the setting of a member variable makes it undesirable, I chose the static option since the connections are cached and so there is not a lot of time spent trying to establish the connection. We could set the member variable in the constructor but that dilutes the meaning of setTimeoutConnection (even if it may not actual use case to set it more than once). Bottom line I thought this way was the cleanest implementation.
If you still think the member variable is required please let me know if you want to set it once in the constructor or remove the static qualifier from the function.
Thanks for reviewing the patch,
Rob

Suresh with resect to

I would like to leave "final public static" for consistency and file a separate ticket to change all the uses of "final public static" to "public static final".

I would like to leave "final public static" for consistency and file a separate ticket to change all the uses of "final public static" to "public static final".

The code has mix of both public static final and other non standard variants. So the new code could just use the right convention. But I will leave it up to you. +1 for making the code consistent in a separate jira.

We could set the member variable in the constructor but that dilutes the meaning of setTimeoutConnection (even if it may not actual use case to set it more than once).

The setConnectionTimeout() is setting a parameter in Configuration object and has nothing to with Client class, right? So, I fail to understand the above point.

The way I see it, Client gets Configuration in the constructor. That is the only point in time, the connection timeout for a client is decided. This is formalized also with member variable conf declared as final. Given that I do not understand why the timeout cannot be set in a final member variable of Client, to clearly show that it is only set once during creation/construction time.

Suresh,
Thanks for your input. This patch uses the correct convention and has the final member variable.

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12565027/HADOOP-9106v4.patch
against trunk revision .

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+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 unit tests in hadoop-common-project/hadoop-common.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2051//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2051//console

This message is automatically generated.

I committed the patch to branch-2 and trunk.

Thanks you Robert!

Integrated in Hadoop-trunk-Commit #3246 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3246/)
HADOOP-9106. Allow configuration of IPC connect timeout. Contributed by Rober Parker. (Revision 1433747)

Result = SUCCESS
suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1433747
Files :

• /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeysPublic.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Client.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/resources/core-default.xml
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestIPC.java

Integrated in Hadoop-Yarn-trunk #98 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/98/)
HADOOP-9106. Allow configuration of IPC connect timeout. Contributed by Rober Parker. (Revision 1433747)

Result = SUCCESS
suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1433747
Files :

• /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeysPublic.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Client.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/resources/core-default.xml
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestIPC.java

Integrated in Hadoop-Hdfs-trunk #1287 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1287/)
HADOOP-9106. Allow configuration of IPC connect timeout. Contributed by Rober Parker. (Revision 1433747)

Result = FAILURE
suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1433747
Files :

• /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeysPublic.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Client.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/resources/core-default.xml
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestIPC.java

Integrated in Hadoop-Mapreduce-trunk #1315 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1315/)
HADOOP-9106. Allow configuration of IPC connect timeout. Contributed by Rober Parker. (Revision 1433747)

Result = FAILURE
suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1433747
Files :

• /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeysPublic.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Client.java
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/resources/core-default.xml
• /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestIPC.java