[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13900135#comment-13900135 ] Devaraj Das commented on HBASE-10490: - I did notice that a 'notifyAll' call was missing in cleanupCalls, but that may be redundant given that in setException there is a notify already being done. Please confirm.. Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch, 10490.v2.patch, 10490.v3.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13900141#comment-13900141 ] Nicolas Liochon commented on HBASE-10490: - bq. redundant given that in setException there is a notify already being done yes, exactly. Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch, 10490.v2.patch, 10490.v3.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13900161#comment-13900161 ] Nicolas Liochon commented on HBASE-10490: - v5 is what I will commit. Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch, 10490.v2.patch, 10490.v3.patch, 10490.v5.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[
https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13900218#comment-13900218
]
Hadoop QA commented on HBASE-10490:
---
{color:red}-1 overall{color}. Here are the results of testing the latest
attachment
http://issues.apache.org/jira/secure/attachment/12628707/10490.v5.patch
against trunk revision .
ATTACHMENT ID: 12628707
{color:green}+1 @author{color}. The patch does not contain any @author
tags.
{color:green}+1 tests included{color}. The patch appears to include 9 new
or modified tests.
{color:green}+1 hadoop1.0{color}. The patch compiles against the hadoop
1.0 profile.
{color:green}+1 hadoop1.1{color}. The patch compiles against the hadoop
1.1 profile.
{color:green}+1 javadoc{color}. The javadoc tool did not generate any
warning messages.
{color:green}+1 javac{color}. The applied patch does not increase the
total number of javac compiler warnings.
{color:red}-1 findbugs{color}. The patch appears to introduce 1 new
Findbugs (version 1.3.9) warnings.
{color:green}+1 release audit{color}. The applied patch does not increase
the total number of release audit warnings.
{color:green}+1 lineLengths{color}. The patch does not introduce lines
longer than 100
{color:red}-1 site{color}. The patch appears to cause mvn site goal to
fail.
{color:green}+1 core tests{color}. The patch passed unit tests in .
Test results:
https://builds.apache.org/job/PreCommit-HBASE-Build/8680//testReport/
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8680//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8680//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8680//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8680//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8680//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8680//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8680//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8680//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8680//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
Console output:
https://builds.apache.org/job/PreCommit-HBASE-Build/8680//console
This message is automatically generated.
Simplify RpcClient code
---
Key: HBASE-10490
URL: https://issues.apache.org/jira/browse/HBASE-10490
Project: HBase
Issue Type: Bug
Components: Client
Affects Versions: 0.99.0
Reporter: Nicolas Liochon
Assignee: Nicolas Liochon
Fix For: 0.99.0
Attachments: 10490.v1.patch, 10490.v2.patch, 10490.v3.patch,
10490.v5.patch
The code is complex. Here is a set of proposed changes, for trunk:
1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception.
I expect that we always have a rpcTimeout. So we can remove the code.
2) remove the sendPing: instead, just close the connection if it's not used
for a while, instead of trying to ping the server.
3) remove maxIddle time: to avoid the confusion if someone has overwritten
the conf.
4) remove shouldCloseConnection: it was more or less synchronized with
closeException. Having a single variable instead of two avoids the synchro
5) remove lastActivity: instead of trying to have an exact timeout, just kill
the connection after some time. lastActivity could be set to wrong values if
the server was slow to answer.
6) hopefully, a better management of the exception; we don't use the close
exception of someone else as an input for another one. Same goes for
interruption.
I may have something wrong in the code. I will review it myself again.
Feedback welcome, especially on the ping removal: I hope I got all the use
cases.
--
This message was sent by Atlassian JIRA
(v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[
https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13900346#comment-13900346
]
Hadoop QA commented on HBASE-10490:
---
{color:red}-1 overall{color}. Here are the results of testing the latest
attachment
http://issues.apache.org/jira/secure/attachment/12628753/10490.v6.patch
against trunk revision .
ATTACHMENT ID: 12628753
{color:green}+1 @author{color}. The patch does not contain any @author
tags.
{color:green}+1 tests included{color}. The patch appears to include 9 new
or modified tests.
{color:red}-1 patch{color}. The patch command could not apply the patch.
Console output:
https://builds.apache.org/job/PreCommit-HBASE-Build/8681//console
This message is automatically generated.
Simplify RpcClient code
---
Key: HBASE-10490
URL: https://issues.apache.org/jira/browse/HBASE-10490
Project: HBase
Issue Type: Bug
Components: Client
Affects Versions: 0.99.0
Reporter: Nicolas Liochon
Assignee: Nicolas Liochon
Fix For: 0.99.0
Attachments: 10490.v1.patch, 10490.v2.patch, 10490.v3.patch,
10490.v5.patch, 10490.v6.patch
The code is complex. Here is a set of proposed changes, for trunk:
1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception.
I expect that we always have a rpcTimeout. So we can remove the code.
2) remove the sendPing: instead, just close the connection if it's not used
for a while, instead of trying to ping the server.
3) remove maxIddle time: to avoid the confusion if someone has overwritten
the conf.
4) remove shouldCloseConnection: it was more or less synchronized with
closeException. Having a single variable instead of two avoids the synchro
5) remove lastActivity: instead of trying to have an exact timeout, just kill
the connection after some time. lastActivity could be set to wrong values if
the server was slow to answer.
6) hopefully, a better management of the exception; we don't use the close
exception of someone else as an input for another one. Same goes for
interruption.
I may have something wrong in the code. I will review it myself again.
Feedback welcome, especially on the ping removal: I hope I got all the use
cases.
--
This message was sent by Atlassian JIRA
(v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13900361#comment-13900361 ] Nicolas Liochon commented on HBASE-10490: - The remaining issues are: - the broken rpc timeout - the sometimes strange synchronization But it's already a nice progress. Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch, 10490.v2.patch, 10490.v3.patch, 10490.v5.patch, 10490.v6.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13900440#comment-13900440 ] Hudson commented on HBASE-10490: SUCCESS: Integrated in HBase-TRUNK #4918 (See [https://builds.apache.org/job/HBase-TRUNK/4918/]) HBASE-10490 Simplify RpcClient code (nkeywal: rev 1567919) * /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/RpcClient.java * /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java * /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRollAbort.java * /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch, 10490.v2.patch, 10490.v3.patch, 10490.v5.patch, 10490.v6.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13900994#comment-13900994 ] Hudson commented on HBASE-10490: FAILURE: Integrated in HBase-TRUNK-on-Hadoop-1.1 #89 (See [https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-1.1/89/]) HBASE-10490 Simplify RpcClient code (nkeywal: rev 1567919) * /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/RpcClient.java * /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java * /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRollAbort.java * /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch, 10490.v2.patch, 10490.v3.patch, 10490.v5.patch, 10490.v6.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13901007#comment-13901007 ] Enis Soztutar commented on HBASE-10490: --- I've also committed this to hbase-10070 branch. We need it there. Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0, hbase-10070 Attachments: 10490.v1.patch, 10490.v2.patch, 10490.v3.patch, 10490.v5.patch, 10490.v6.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13899132#comment-13899132 ] Nicolas Liochon commented on HBASE-10490: - As well, a rpcTimeout set to 0 might still work, or at least still work as it used to. The code around timeout is still difficult. I kept the ThreadLocal, but this code scares me, and clearly does not work for all cases. It's an issue for me, because when something goes wrong here I'm never sure it does not come from a race condition. As well, for example, we can set very low timeouts. So the cleanup on timeout is not in this jira, but will have to be done a day for sure. Anyway, here a a v2 with the typo fixed and the removal of the closeReason. Thanks for the feedback Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13899138#comment-13899138 ] Nicolas Liochon commented on HBASE-10490: - btw, The v2 removes ~140 LoC in RpcClient (~10%) Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch, 10490.v2.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13899463#comment-13899463 ] Nicolas Liochon commented on HBASE-10490: - I've have 3 +1, but it could more on the idea of removing the ping. If the hadoop-qa is fine, I plan to commit this tomorrow, please tell me if you want to do a deeper review and need more time. Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch, 10490.v2.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[
https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13899484#comment-13899484
]
Hadoop QA commented on HBASE-10490:
---
{color:red}-1 overall{color}. Here are the results of testing the latest
attachment
http://issues.apache.org/jira/secure/attachment/12628486/10490.v2.patch
against trunk revision .
ATTACHMENT ID: 12628486
{color:green}+1 @author{color}. The patch does not contain any @author
tags.
{color:green}+1 tests included{color}. The patch appears to include 9 new
or modified tests.
{color:green}+1 hadoop1.0{color}. The patch compiles against the hadoop
1.0 profile.
{color:green}+1 hadoop1.1{color}. The patch compiles against the hadoop
1.1 profile.
{color:green}+1 javadoc{color}. The javadoc tool did not generate any
warning messages.
{color:green}+1 javac{color}. The applied patch does not increase the
total number of javac compiler warnings.
{color:green}+1 findbugs{color}. The patch does not introduce any new
Findbugs (version 1.3.9) warnings.
{color:green}+1 release audit{color}. The applied patch does not increase
the total number of release audit warnings.
{color:green}+1 lineLengths{color}. The patch does not introduce lines
longer than 100
{color:red}-1 site{color}. The patch appears to cause mvn site goal to
fail.
{color:red}-1 core tests{color}. The patch failed these unit tests:
org.apache.hadoop.hbase.client.TestMultiParallel
Test results:
https://builds.apache.org/job/PreCommit-HBASE-Build/8670//testReport/
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8670//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8670//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8670//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8670//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8670//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8670//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8670//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8670//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8670//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
Console output:
https://builds.apache.org/job/PreCommit-HBASE-Build/8670//console
This message is automatically generated.
Simplify RpcClient code
---
Key: HBASE-10490
URL: https://issues.apache.org/jira/browse/HBASE-10490
Project: HBase
Issue Type: Bug
Components: Client
Affects Versions: 0.99.0
Reporter: Nicolas Liochon
Assignee: Nicolas Liochon
Fix For: 0.99.0
Attachments: 10490.v1.patch, 10490.v2.patch
The code is complex. Here is a set of proposed changes, for trunk:
1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception.
I expect that we always have a rpcTimeout. So we can remove the code.
2) remove the sendPing: instead, just close the connection if it's not used
for a while, instead of trying to ping the server.
3) remove maxIddle time: to avoid the confusion if someone has overwritten
the conf.
4) remove shouldCloseConnection: it was more or less synchronized with
closeException. Having a single variable instead of two avoids the synchro
5) remove lastActivity: instead of trying to have an exact timeout, just kill
the connection after some time. lastActivity could be set to wrong values if
the server was slow to answer.
6) hopefully, a better management of the exception; we don't use the close
exception of someone else as an input for another one. Same goes for
interruption.
I may have something wrong in the code. I will review it myself again.
Feedback welcome, especially on the ping removal: I hope I got all the use
cases.
--
This message was sent by Atlassian JIRA
(v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13899689#comment-13899689 ] Nicolas Liochon commented on HBASE-10490: - The issue raised by hadoop-qa was real. the lock order is Connection instance - DataOutputStream. Doing it in the other side leads to dead locks.. Fixed. I put the calls counter in an atomic counter as well. Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch, 10490.v2.patch, 10490.v3.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13899757#comment-13899757 ] stack commented on HBASE-10490: --- Skimmed through. Looks great. +1 if all tests pass. Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch, 10490.v2.patch, 10490.v3.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13899758#comment-13899758 ] stack commented on HBASE-10490: --- I love in particular the amount of code removed Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch, 10490.v2.patch, 10490.v3.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[
https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13899802#comment-13899802
]
Hadoop QA commented on HBASE-10490:
---
{color:red}-1 overall{color}. Here are the results of testing the latest
attachment
http://issues.apache.org/jira/secure/attachment/12628585/10490.v3.patch
against trunk revision .
ATTACHMENT ID: 12628585
{color:green}+1 @author{color}. The patch does not contain any @author
tags.
{color:green}+1 tests included{color}. The patch appears to include 9 new
or modified tests.
{color:green}+1 hadoop1.0{color}. The patch compiles against the hadoop
1.0 profile.
{color:green}+1 hadoop1.1{color}. The patch compiles against the hadoop
1.1 profile.
{color:green}+1 javadoc{color}. The javadoc tool did not generate any
warning messages.
{color:green}+1 javac{color}. The applied patch does not increase the
total number of javac compiler warnings.
{color:red}-1 findbugs{color}. The patch appears to introduce 2 new
Findbugs (version 1.3.9) warnings.
{color:green}+1 release audit{color}. The applied patch does not increase
the total number of release audit warnings.
{color:green}+1 lineLengths{color}. The patch does not introduce lines
longer than 100
{color:red}-1 site{color}. The patch appears to cause mvn site goal to
fail.
{color:green}+1 core tests{color}. The patch passed unit tests in .
Test results:
https://builds.apache.org/job/PreCommit-HBASE-Build/8676//testReport/
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8676//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8676//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8676//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8676//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8676//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8676//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8676//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8676//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8676//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
Console output:
https://builds.apache.org/job/PreCommit-HBASE-Build/8676//console
This message is automatically generated.
Simplify RpcClient code
---
Key: HBASE-10490
URL: https://issues.apache.org/jira/browse/HBASE-10490
Project: HBase
Issue Type: Bug
Components: Client
Affects Versions: 0.99.0
Reporter: Nicolas Liochon
Assignee: Nicolas Liochon
Fix For: 0.99.0
Attachments: 10490.v1.patch, 10490.v2.patch, 10490.v3.patch
The code is complex. Here is a set of proposed changes, for trunk:
1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception.
I expect that we always have a rpcTimeout. So we can remove the code.
2) remove the sendPing: instead, just close the connection if it's not used
for a while, instead of trying to ping the server.
3) remove maxIddle time: to avoid the confusion if someone has overwritten
the conf.
4) remove shouldCloseConnection: it was more or less synchronized with
closeException. Having a single variable instead of two avoids the synchro
5) remove lastActivity: instead of trying to have an exact timeout, just kill
the connection after some time. lastActivity could be set to wrong values if
the server was slow to answer.
6) hopefully, a better management of the exception; we don't use the close
exception of someone else as an input for another one. Same goes for
interruption.
I may have something wrong in the code. I will review it myself again.
Feedback welcome, especially on the ping removal: I hope I got all the use
cases.
--
This message was sent by Atlassian JIRA
(v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13899826#comment-13899826 ] Nicolas Liochon commented on HBASE-10490: - The warnings are false positives that can be fixed by with the ad hoc findbugs setting. I will do that on commit/ Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch, 10490.v2.patch, 10490.v3.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[
https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13899962#comment-13899962
]
Enis Soztutar commented on HBASE-10490:
---
It looks ok to me, but I am far from being an expert on the rpc / client side.
Is there any reason why we are using negative call id's:
{code}
this.id = callIdCnt.getAndDecrement();
{code}
Simplify RpcClient code
---
Key: HBASE-10490
URL: https://issues.apache.org/jira/browse/HBASE-10490
Project: HBase
Issue Type: Bug
Components: Client
Affects Versions: 0.99.0
Reporter: Nicolas Liochon
Assignee: Nicolas Liochon
Fix For: 0.99.0
Attachments: 10490.v1.patch, 10490.v2.patch, 10490.v3.patch
The code is complex. Here is a set of proposed changes, for trunk:
1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception.
I expect that we always have a rpcTimeout. So we can remove the code.
2) remove the sendPing: instead, just close the connection if it's not used
for a while, instead of trying to ping the server.
3) remove maxIddle time: to avoid the confusion if someone has overwritten
the conf.
4) remove shouldCloseConnection: it was more or less synchronized with
closeException. Having a single variable instead of two avoids the synchro
5) remove lastActivity: instead of trying to have an exact timeout, just kill
the connection after some time. lastActivity could be set to wrong values if
the server was slow to answer.
6) hopefully, a better management of the exception; we don't use the close
exception of someone else as an input for another one. Same goes for
interruption.
I may have something wrong in the code. I will review it myself again.
Feedback welcome, especially on the ping removal: I hope I got all the use
cases.
--
This message was sent by Atlassian JIRA
(v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13899966#comment-13899966 ] Nicolas Liochon commented on HBASE-10490: - It's a typo. It's interesting that it works actually. Thanks for catching this. I will fix this on commit. Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch, 10490.v2.patch, 10490.v3.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13897617#comment-13897617 ] Devaraj Das commented on HBASE-10490: - Nice cleanup, but hopefully we can make sure the changes doesn't break any assumption in the RPC layer. The 'ping' removal stood out while I was doing a review. Am wondering if the server side works well with this change.. i mean could this happen (1) client sends an RPC (2) server got to it but the request is taking a long time to process (3) meanwhile the server sees the connection as idle and closes it (since no ping came) The other thing is if the client's intended socket timeout is 0 (infinite timeout), wondering if the ping is relevant then to prevent server from closing the connection on incomplete/not-yet-responded RPCs. Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[
https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13897660#comment-13897660
]
Nicolas Liochon commented on HBASE-10490:
-
Nice catch. The max idle time is actually used on the server as well. But it
does not really work, because the default are different:
server: maxIdleTime = 2*conf.getInt(ipc.client.connection.maxidletime, 1000);
client: maxIdleTime = conf.getInt(hbase.ipc.client.connection.maxidletime,
1); //10s
So it means that the server disconnects any client that have not spoken for 2
seconds; while the client pings every 10 seconds.
not as well that one is prefixed by 'hbase.' while the other is not. In 2008
they were sharing the same name then it diverged.
I suppose the code on the server doesn't do much because of this:
{code}
protected int thresholdIdleConnections; // the number of idle
// connections after which we
// will start cleaning up idle
// connections
{code}
thresholdIdleConnections is defaulted to 4000. Likely it never happens, and if
it was happening it would not work because of the difference in default values.
I suppose the best way of doing this is: order the connection not used for at
least x seconds, kills some of the oldest.
But, we can say as well that if we're satisfied with the way the server behaves
today, we can remove the ping on the client without changing anything in the
server: the behavior won't change.
Simplify RpcClient code
---
Key: HBASE-10490
URL: https://issues.apache.org/jira/browse/HBASE-10490
Project: HBase
Issue Type: Bug
Components: Client
Affects Versions: 0.99.0
Reporter: Nicolas Liochon
Assignee: Nicolas Liochon
Fix For: 0.99.0
Attachments: 10490.v1.patch
The code is complex. Here is a set of proposed changes, for trunk:
1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception.
I expect that we always have a rpcTimeout. So we can remove the code.
2) remove the sendPing: instead, just close the connection if it's not used
for a while, instead of trying to ping the server.
3) remove maxIddle time: to avoid the confusion if someone has overwritten
the conf.
4) remove shouldCloseConnection: it was more or less synchronized with
closeException. Having a single variable instead of two avoids the synchro
5) remove lastActivity: instead of trying to have an exact timeout, just kill
the connection after some time. lastActivity could be set to wrong values if
the server was slow to answer.
6) hopefully, a better management of the exception; we don't use the close
exception of someone else as an input for another one. Same goes for
interruption.
I may have something wrong in the code. I will review it myself again.
Feedback welcome, especially on the ping removal: I hope I got all the use
cases.
--
This message was sent by Atlassian JIRA
(v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13898298#comment-13898298 ] Devaraj Das commented on HBASE-10490: - I can't say for sure if in HBase anyone configures infinite timeout (rpcTimeout = 0) on the sockets but the pingery would have protected the client if it wanted to wait for a while in the situations where the server is busy. So if the rpcTimeout is passed as zero, the socket timeout is set to the ping interval. That means the client won't retry when the timeout happens. It'll just send a ping to figure out whether the server is still alive. If so, then it'll continue to wait (as opposed to resending the request). But I agree that if no one uses rpcTimeout = 0, we could remove the ping stuff. Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13898304#comment-13898304 ] Devaraj Das commented on HBASE-10490: - rpcTimeout in my last comment refers to the configured value of hbase.rpc.timeout. Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13898396#comment-13898396 ] stack commented on HBASE-10490: --- bq. meanwhile the server sees the connection as idle and closes it If server is taking timeout to reply, the client will be gone anyways? If request is taking tens of seconds, we should kill it? If this is expected, up the socket timeout? bq. we can remove the ping on the client without changing anything in the server: We could. I like purging ping altogether (unless I'm wrong above) since it puts a particular shape on how we process the incoming requests (look for the special -1 length indicator and short circuit if a ping) and would like this cleaned up so easier putting in another request handling (e.g. async). bq. But I agree that if no one uses rpcTimeout = 0, we could remove the ping stuff. Lets beat anyone who has their rpcTimeout to 0. Smile (That said, I have vague recollection that rpcTimeout==0 was how we defaulted at one time so let me go beat myself in the past) Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13898417#comment-13898417 ] Devaraj Das commented on HBASE-10490: - bq. Lets beat anyone who has their rpcTimeout to 0. If it's as simple as beating them up, +1 (though I would advise you to not beat yourself up just yet, Stack [smile]). Applications could break because of the fact that a timeout of 0 won't be supported any more (maybe log a big warning if we detect this in the RPC client). And if there is agreement, this should be one of the things we stop supporting in the upcoming 1.0. Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13898435#comment-13898435 ] Lars Hofhansl commented on HBASE-10490: --- +1 on removing the pingery. Just this discussion shows how convoluted it has become. Even with rpcTimeout = 0, would clients actually break? Wouldn't they just reconnect? (I might be confused) Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13898447#comment-13898447 ] Devaraj Das commented on HBASE-10490: - bq. Even with rpcTimeout = 0, would clients actually break? Wouldn't they just reconnect? Most likely, they will. My point was that there is some change in semantics. Clients might be handling them well enough already. Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13898468#comment-13898468 ] stack commented on HBASE-10490: --- If someone wants timeout=0, and they want to avoid a beating, they could do timeout=Long.MAX_VALUE? I can't think of a place where timeout=0 would make any sense. Good stuff. Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13896796#comment-13896796 ] stack commented on HBASE-10490: --- +1 on removing the ping code. Spelling minIddleTimeBeforeClose I know this is not you, but this mechanism seems fragile +protected final AtomicReferenceIOException closeReason = new AtomicReferenceIOException(); i.e. keeping around the exception Why do you think the code used try to contain exceptions? Now you let them out. Good stuff [~nkeywal] Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[ https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13896802#comment-13896802 ] Nicolas Liochon commented on HBASE-10490: - bq. Spelling minIddleTimeBeforeClose Sure. bq. i.e. keeping around the exception We're clean now: we now use it only in logs and as initCause in the exceptions we throw. So we can as well fully remove it (basically, between the boolean and the exception, I kept the exception, but keeping only the boolean should be fine as well). Simplify RpcClient code --- Key: HBASE-10490 URL: https://issues.apache.org/jira/browse/HBASE-10490 Project: HBase Issue Type: Bug Components: Client Affects Versions: 0.99.0 Reporter: Nicolas Liochon Assignee: Nicolas Liochon Fix For: 0.99.0 Attachments: 10490.v1.patch The code is complex. Here is a set of proposed changes, for trunk: 1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception. I expect that we always have a rpcTimeout. So we can remove the code. 2) remove the sendPing: instead, just close the connection if it's not used for a while, instead of trying to ping the server. 3) remove maxIddle time: to avoid the confusion if someone has overwritten the conf. 4) remove shouldCloseConnection: it was more or less synchronized with closeException. Having a single variable instead of two avoids the synchro 5) remove lastActivity: instead of trying to have an exact timeout, just kill the connection after some time. lastActivity could be set to wrong values if the server was slow to answer. 6) hopefully, a better management of the exception; we don't use the close exception of someone else as an input for another one. Same goes for interruption. I may have something wrong in the code. I will review it myself again. Feedback welcome, especially on the ping removal: I hope I got all the use cases. -- This message was sent by Atlassian JIRA (v6.1.5#6160)
[jira] [Commented] (HBASE-10490) Simplify RpcClient code
[
https://issues.apache.org/jira/browse/HBASE-10490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13896872#comment-13896872
]
Hadoop QA commented on HBASE-10490:
---
{color:red}-1 overall{color}. Here are the results of testing the latest
attachment
http://issues.apache.org/jira/secure/attachment/12628010/10490.v1.patch
against trunk revision .
ATTACHMENT ID: 12628010
{color:green}+1 @author{color}. The patch does not contain any @author
tags.
{color:green}+1 tests included{color}. The patch appears to include 9 new
or modified tests.
{color:green}+1 hadoop1.0{color}. The patch compiles against the hadoop
1.0 profile.
{color:green}+1 hadoop1.1{color}. The patch compiles against the hadoop
1.1 profile.
{color:green}+1 javadoc{color}. The javadoc tool did not generate any
warning messages.
{color:green}+1 javac{color}. The applied patch does not increase the
total number of javac compiler warnings.
{color:green}+1 findbugs{color}. The patch does not introduce any new
Findbugs (version 1.3.9) warnings.
{color:green}+1 release audit{color}. The applied patch does not increase
the total number of release audit warnings.
{color:green}+1 lineLengths{color}. The patch does not introduce lines
longer than 100
{color:red}-1 site{color}. The patch appears to cause mvn site goal to
fail.
{color:red}-1 core tests{color}. The patch failed these unit tests:
org.apache.hadoop.hbase.client.TestMultiParallel
org.apache.hadoop.hbase.replication.TestReplicationKillSlaveRS
Test results:
https://builds.apache.org/job/PreCommit-HBASE-Build/8651//testReport/
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8651//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8651//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8651//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8651//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8651//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8651//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8651//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8651//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
Findbugs warnings:
https://builds.apache.org/job/PreCommit-HBASE-Build/8651//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
Console output:
https://builds.apache.org/job/PreCommit-HBASE-Build/8651//console
This message is automatically generated.
Simplify RpcClient code
---
Key: HBASE-10490
URL: https://issues.apache.org/jira/browse/HBASE-10490
Project: HBase
Issue Type: Bug
Components: Client
Affects Versions: 0.99.0
Reporter: Nicolas Liochon
Assignee: Nicolas Liochon
Fix For: 0.99.0
Attachments: 10490.v1.patch
The code is complex. Here is a set of proposed changes, for trunk:
1) remove PingInputStream. if rpcTimeout 0 it just rethrows the exception.
I expect that we always have a rpcTimeout. So we can remove the code.
2) remove the sendPing: instead, just close the connection if it's not used
for a while, instead of trying to ping the server.
3) remove maxIddle time: to avoid the confusion if someone has overwritten
the conf.
4) remove shouldCloseConnection: it was more or less synchronized with
closeException. Having a single variable instead of two avoids the synchro
5) remove lastActivity: instead of trying to have an exact timeout, just kill
the connection after some time. lastActivity could be set to wrong values if
the server was slow to answer.
6) hopefully, a better management of the exception; we don't use the close
exception of someone else as an input for another one. Same goes for
interruption.
I may have something wrong in the code. I will review it myself again.
Feedback welcome, especially on the ping removal: I hope I got all the use
cases.
--
This message was sent by Atlassian JIRA
(v6.1.5#6160)
