Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.99.0
    • Fix Version/s: 0.99.0, hbase-10070
    • Component/s: Client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      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.

      1. 10490.v6.patch
        34 kB
        Nicolas Liochon
      2. 10490.v5.patch
        34 kB
        Nicolas Liochon
      3. 10490.v3.patch
        33 kB
        Nicolas Liochon
      4. 10490.v2.patch
        31 kB
        Nicolas Liochon
      5. 10490.v1.patch
        30 kB
        Nicolas Liochon

        Issue Links

          Activity

          Hide
          stack added a comment -

          +1 on removing the ping code.

          Spelling minIddleTimeBeforeClose

          I know this is not you, but this mechanism seems fragile

          + protected final AtomicReference<IOException> closeReason = new AtomicReference<IOException>();

          i.e. keeping around the exception

          Why do you think the code used try to contain exceptions? Now you let them out.

          Good stuff Nicolas Liochon

          Show
          stack added a comment - +1 on removing the ping code. Spelling minIddleTimeBeforeClose I know this is not you, but this mechanism seems fragile + protected final AtomicReference<IOException> closeReason = new AtomicReference<IOException>(); i.e. keeping around the exception Why do you think the code used try to contain exceptions? Now you let them out. Good stuff Nicolas Liochon
          Hide
          Nicolas Liochon added a comment -

          Spelling minIddleTimeBeforeClose

          Sure.

          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).

          Show
          Nicolas Liochon added a comment - Spelling minIddleTimeBeforeClose Sure. 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).
          Hide
          Hadoop QA added a comment -

          -1 overall. 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

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

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

          +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

          +1 hadoop1.1. The patch compiles against the hadoop 1.1 profile.

          +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 lineLengths. The patch does not introduce lines longer than 100

          -1 site. The patch appears to cause mvn site goal to fail.

          -1 core tests. 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.

          Show
          Hadoop QA added a comment - -1 overall . 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 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 9 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop1.1 . The patch compiles against the hadoop 1.1 profile. +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 lineLengths . The patch does not introduce lines longer than 100 -1 site . The patch appears to cause mvn site goal to fail. -1 core tests . 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.
          Hide
          Devaraj Das added a comment -

          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.

          Show
          Devaraj Das added a comment - 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.
          Hide
          Nicolas Liochon added a comment -

          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", 10000); //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:

            protected int thresholdIdleConnections;         // the number of idle
                                                            // connections after which we
                                                            // will start cleaning up idle
                                                            // connections
          

          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.

          Show
          Nicolas Liochon added a comment - 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", 10000); //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: protected int thresholdIdleConnections; // the number of idle // connections after which we // will start cleaning up idle // connections 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.
          Hide
          Devaraj Das added a comment -

          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.

          Show
          Devaraj Das added a comment - 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.
          Hide
          Devaraj Das added a comment -

          rpcTimeout in my last comment refers to the configured value of hbase.rpc.timeout.

          Show
          Devaraj Das added a comment - rpcTimeout in my last comment refers to the configured value of hbase.rpc.timeout.
          Hide
          stack added a comment -

          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?

          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).

          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)

          Show
          stack added a comment - 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? 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). 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)
          Hide
          Devaraj Das added a comment -

          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.

          Show
          Devaraj Das added a comment - 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.
          Hide
          Lars Hofhansl added a comment -

          +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)

          Show
          Lars Hofhansl added a comment - +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)
          Hide
          Devaraj Das added a comment -

          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.

          Show
          Devaraj Das added a comment - 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.
          Hide
          stack added a comment -

          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.

          Show
          stack added a comment - 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.
          Hide
          Nicolas Liochon added a comment -

          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....

          Show
          Nicolas Liochon added a comment - 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....
          Hide
          Nicolas Liochon added a comment -

          btw, The v2 removes ~140 LoC in RpcClient (~10%)

          Show
          Nicolas Liochon added a comment - btw, The v2 removes ~140 LoC in RpcClient (~10%)
          Hide
          Nicolas Liochon added a comment - - edited

          I 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.

          Show
          Nicolas Liochon added a comment - - edited I 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.
          Hide
          Hadoop QA added a comment -

          -1 overall. 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

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

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

          +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

          +1 hadoop1.1. The patch compiles against the hadoop 1.1 profile.

          +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 lineLengths. The patch does not introduce lines longer than 100

          -1 site. The patch appears to cause mvn site goal to fail.

          -1 core tests. 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.

          Show
          Hadoop QA added a comment - -1 overall . 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 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 9 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop1.1 . The patch compiles against the hadoop 1.1 profile. +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 lineLengths . The patch does not introduce lines longer than 100 -1 site . The patch appears to cause mvn site goal to fail. -1 core tests . 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.
          Hide
          Nicolas Liochon added a comment -

          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.

          Show
          Nicolas Liochon added a comment - 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.
          Hide
          stack added a comment -

          Skimmed through. Looks great. +1 if all tests pass.

          Show
          stack added a comment - Skimmed through. Looks great. +1 if all tests pass.
          Hide
          stack added a comment -

          I love in particular the amount of code removed

          Show
          stack added a comment - I love in particular the amount of code removed
          Hide
          Hadoop QA added a comment -

          -1 overall. 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

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

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

          +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

          +1 hadoop1.1. The patch compiles against the hadoop 1.1 profile.

          +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 appears to introduce 2 new Findbugs (version 1.3.9) warnings.

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

          +1 lineLengths. The patch does not introduce lines longer than 100

          -1 site. The patch appears to cause mvn site goal to fail.

          +1 core tests. 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.

          Show
          Hadoop QA added a comment - -1 overall . 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 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 9 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop1.1 . The patch compiles against the hadoop 1.1 profile. +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 appears to introduce 2 new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 -1 site . The patch appears to cause mvn site goal to fail. +1 core tests . 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.
          Hide
          Nicolas Liochon added a comment -

          The warnings are false positives that can be fixed by with the ad hoc findbugs setting. I will do that on commit/

          Show
          Nicolas Liochon added a comment - The warnings are false positives that can be fixed by with the ad hoc findbugs setting. I will do that on commit/
          Hide
          Enis Soztutar added a comment -

          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:

          this.id = callIdCnt.getAndDecrement();
          
          Show
          Enis Soztutar added a comment - 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: this .id = callIdCnt.getAndDecrement();
          Hide
          Nicolas Liochon added a comment -

          It's a typo. It's interesting that it works actually. Thanks for catching
          this. I will fix this on commit.

          Show
          Nicolas Liochon added a comment - It's a typo. It's interesting that it works actually. Thanks for catching this. I will fix this on commit.
          Hide
          Devaraj Das added a comment -

          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..

          Show
          Devaraj Das added a comment - 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..
          Hide
          Nicolas Liochon added a comment -

          redundant given that in setException there is a notify already being done

          yes, exactly.

          Show
          Nicolas Liochon added a comment - redundant given that in setException there is a notify already being done yes, exactly.
          Hide
          Nicolas Liochon added a comment -

          v5 is what I will commit.

          Show
          Nicolas Liochon added a comment - v5 is what I will commit.
          Hide
          Hadoop QA added a comment -

          -1 overall. 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

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

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

          +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

          +1 hadoop1.1. The patch compiles against the hadoop 1.1 profile.

          +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 appears to introduce 1 new Findbugs (version 1.3.9) warnings.

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

          +1 lineLengths. The patch does not introduce lines longer than 100

          -1 site. The patch appears to cause mvn site goal to fail.

          +1 core tests. 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.

          Show
          Hadoop QA added a comment - -1 overall . 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 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 9 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop1.1 . The patch compiles against the hadoop 1.1 profile. +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 appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 -1 site . The patch appears to cause mvn site goal to fail. +1 core tests . 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.
          Hide
          Hadoop QA added a comment -

          -1 overall. 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

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

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

          -1 patch. 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.

          Show
          Hadoop QA added a comment - -1 overall . 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 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 9 new or modified tests. -1 patch . 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.
          Hide
          Nicolas Liochon added a comment -

          Committed, thanks for the reviews, all.

          Show
          Nicolas Liochon added a comment - Committed, thanks for the reviews, all.
          Hide
          Nicolas Liochon added a comment -

          The remaining issues are:

          • the broken rpc timeout
          • the sometimes strange synchronization

          But it's already a nice progress.

          Show
          Nicolas Liochon added a comment - The remaining issues are: the broken rpc timeout the sometimes strange synchronization But it's already a nice progress.
          Hide
          Hudson added a comment -

          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
          Show
          Hudson added a comment - 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
          Hide
          Hudson added a comment -

          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
          Show
          Hudson added a comment - 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
          Hide
          Enis Soztutar added a comment -

          I've also committed this to hbase-10070 branch. We need it there.

          Show
          Enis Soztutar added a comment - I've also committed this to hbase-10070 branch. We need it there.
          Hide
          Enis Soztutar added a comment -

          Closing this issue after 0.99.0 release.

          Show
          Enis Soztutar added a comment - Closing this issue after 0.99.0 release.

            People

            • Assignee:
              Nicolas Liochon
              Reporter:
              Nicolas Liochon
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development