HBase
  1. HBase
  2. HBASE-10606

Bad timeout in RpcRetryingCaller#callWithRetries w/o parameters

    Details

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

      Description

      When we call this method w/o parameters, we don't take into account the configuration, but use the hardcoded default (Integer.MAX).

      If someone was relying on having an infinite timeout whatever the setting, fixing this bug will cause him a surprise. But there is no magic...

      1. 10606.v1.patch
        13 kB
        Nicolas Liochon
      2. 10606.v2.patch
        21 kB
        Nicolas Liochon
      3. 10606.v4.patch
        25 kB
        Nicolas Liochon

        Activity

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

        FAILURE: Integrated in HBase-TRUNK-on-Hadoop-1.1 #100 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-1.1/100/)
        HBASE-10606 Bad timeout in RpcRetryingCaller#callWithRetries w/o parameters (nkeywal: rev 1572124)

        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncProcess.java
        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java
        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientSmallScanner.java
        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java
        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ReversedClientScanner.java
        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RpcRetryingCaller.java
        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/RegionCoprocessorRpcChannel.java
        • /hbase/trunk/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestAsyncProcess.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.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/TestHRegionServerBulkLoad.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRollAbort.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-TRUNK-on-Hadoop-1.1 #100 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-1.1/100/ ) HBASE-10606 Bad timeout in RpcRetryingCaller#callWithRetries w/o parameters (nkeywal: rev 1572124) /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncProcess.java /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientSmallScanner.java /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ReversedClientScanner.java /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RpcRetryingCaller.java /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/RegionCoprocessorRpcChannel.java /hbase/trunk/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestAsyncProcess.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.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/TestHRegionServerBulkLoad.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRollAbort.java
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-TRUNK #4958 (See https://builds.apache.org/job/HBase-TRUNK/4958/)
        HBASE-10606 Bad timeout in RpcRetryingCaller#callWithRetries w/o parameters (nkeywal: rev 1572124)

        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncProcess.java
        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java
        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientSmallScanner.java
        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java
        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ReversedClientScanner.java
        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RpcRetryingCaller.java
        • /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/RegionCoprocessorRpcChannel.java
        • /hbase/trunk/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestAsyncProcess.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.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/TestHRegionServerBulkLoad.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRollAbort.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-TRUNK #4958 (See https://builds.apache.org/job/HBase-TRUNK/4958/ ) HBASE-10606 Bad timeout in RpcRetryingCaller#callWithRetries w/o parameters (nkeywal: rev 1572124) /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncProcess.java /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientSmallScanner.java /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ReversedClientScanner.java /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RpcRetryingCaller.java /hbase/trunk/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/RegionCoprocessorRpcChannel.java /hbase/trunk/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestAsyncProcess.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.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/TestHRegionServerBulkLoad.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRollAbort.java
        Hide
        Nicolas Liochon added a comment -

        Committed v4 to trunk, thanks for the reviews, Nick & Stack.

        Show
        Nicolas Liochon added a comment - Committed v4 to trunk, thanks for the reviews, Nick & Stack.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12631217/10606.v4.patch
        against trunk revision .
        ATTACHMENT ID: 12631217

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

        +1 tests included. The patch appears to include 12 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 mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//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/12631217/10606.v4.patch against trunk revision . ATTACHMENT ID: 12631217 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 12 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 mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8813//console This message is automatically generated.
        Hide
        Nicolas Liochon added a comment -

        My opinion: why include it you don't use it.

        I don't use it because I don't test this part . I wrote the test on HTable, and I used the setter there. But anyway, I removed the setter in the last version.

        By all means, let them bubble up! No over-catching.

        At the end, I changed my mind and put the throwable back, checking for Interrupted exception. At least it's 'as before'.

        Other comments taken into account as well.

        v4 is what i will commit if the tests are green again.

        Show
        Nicolas Liochon added a comment - My opinion: why include it you don't use it. I don't use it because I don't test this part . I wrote the test on HTable, and I used the setter there. But anyway, I removed the setter in the last version. By all means, let them bubble up! No over-catching. At the end, I changed my mind and put the throwable back, checking for Interrupted exception. At least it's 'as before'. Other comments taken into account as well. v4 is what i will commit if the tests are green again.
        Hide
        Nick Dimiduk added a comment -

        I can remove it for sure.

        My opinion: why include it you don't use it.

        There is no exception declared. [snip] but I like to remove catch throwables

        By all means, let them bubble up! No over-catching.

        Show
        Nick Dimiduk added a comment - I can remove it for sure. My opinion: why include it you don't use it. There is no exception declared. [snip] but I like to remove catch throwables By all means, let them bubble up! No over-catching.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12630987/10606.v2.patch
        against trunk revision .
        ATTACHMENT ID: 12630987

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

        +1 tests included. The patch appears to include 6 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 mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//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/12630987/10606.v2.patch against trunk revision . ATTACHMENT ID: 12630987 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 6 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 mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8802//console This message is automatically generated.
        Hide
        Nicolas Liochon added a comment -

        Nick, our comments crossed path.
        What do you think for the setter? It's not really a 'setter bypassing immutability', no? I can remove it for sure.

        This swallowed exception will now burp up in all kinds of places (AssignmentManager, Handlers, &c), right?

        There is no exception declared.
        We're overcatching for sure, for example we would shallow interruption if they were there... I can remove it as well from the patch, but I like to remove catch throwables .

        What is "cto"? "callTimeOut"?

        Yes, I will fix this.

        nit, ws:

        Will fix as well

        Show
        Nicolas Liochon added a comment - Nick, our comments crossed path. What do you think for the setter? It's not really a 'setter bypassing immutability', no? I can remove it for sure. This swallowed exception will now burp up in all kinds of places (AssignmentManager, Handlers, &c), right? There is no exception declared. We're overcatching for sure, for example we would shallow interruption if they were there... I can remove it as well from the patch, but I like to remove catch throwables . What is "cto"? "callTimeOut"? Yes, I will fix this. nit, ws: Will fix as well
        Hide
        Nick Dimiduk added a comment -

        Sorry, meant to conclude that with +1

        Show
        Nick Dimiduk added a comment - Sorry, meant to conclude that with +1
        Hide
        Nicolas Liochon added a comment -

        That's the test result for v1. Still, I'm surprised it made it! I will run the tests with v2 as well.

        Can't we just set it on construction and then be done w/ it rather than provide exotic options?

        I've used the same pattern as in HTable. I think it's useful, as you can do a call with a different timeout if you need. That's the way I wrote a test for HTable at least.

        private void beforeCall() {

        It's replaced by the getRemaining(). It's better, as there is no state anymore in the object.

        If we are removing some infinite timeout, good; commit and lets live w/ the consequence.

        Yeah, there will some (bad) consequences for sure...

        I'm waiting for the result of v2, I will commit if it's ok. Thanks a lot for the review.

        Show
        Nicolas Liochon added a comment - That's the test result for v1. Still, I'm surprised it made it! I will run the tests with v2 as well. Can't we just set it on construction and then be done w/ it rather than provide exotic options? I've used the same pattern as in HTable. I think it's useful, as you can do a call with a different timeout if you need. That's the way I wrote a test for HTable at least. private void beforeCall() { It's replaced by the getRemaining(). It's better, as there is no state anymore in the object. If we are removing some infinite timeout, good; commit and lets live w/ the consequence. Yeah, there will some (bad) consequences for sure... I'm waiting for the result of v2, I will commit if it's ok. Thanks a lot for the review.
        Hide
        Nick Dimiduk added a comment -

        Can't we just set it on construction and then be done w/ it rather than provide exotic options?

        +1 on doing away with setters for state like this.

        -        try {
        -          scanner.close();
        -        } catch (Throwable t) {
        -          LOG.debug("Got exception in closing the result scanner", t);
        -        }
        +        scanner.close();
        

        This is a little scary to me. This swallowed exception will now burp up in all kinds of places (AssignmentManager, Handlers, &c), right?

        -        public MultiResponse callWithoutRetries( RetryingCallable<MultiResponse> callable)
        +        public MultiResponse callWithoutRetries(RetryingCallable<MultiResponse> callable, int cto)
        

        What is "cto"? "callTimeOut"?

        nit, ws:

        +    this.operationTimeout =   conf.getInt(HConstants.HBASE_CLIENT_OPERATION_TIMEOUT,
        
        Show
        Nick Dimiduk added a comment - Can't we just set it on construction and then be done w/ it rather than provide exotic options? +1 on doing away with setters for state like this. - try { - scanner.close(); - } catch (Throwable t) { - LOG.debug("Got exception in closing the result scanner", t); - } + scanner.close(); This is a little scary to me. This swallowed exception will now burp up in all kinds of places (AssignmentManager, Handlers, &c), right? - public MultiResponse callWithoutRetries( RetryingCallable<MultiResponse> callable) + public MultiResponse callWithoutRetries(RetryingCallable<MultiResponse> callable, int cto) What is "cto"? "callTimeOut"? nit, ws: + this.operationTimeout = conf.getInt(HConstants.HBASE_CLIENT_OPERATION_TIMEOUT,
        Hide
        stack added a comment -

        We need this?

        + public void setOperationTimeout(int operationTimeout) {

        Can't we just set it on construction and then be done w/ it rather than provide exotic options?

        This goes away? Not needed any more? Not even as a noop?

        • private void beforeCall() {

        Patch looks great. +1. If we are removing some infinite timeout, good; commit and lets live w/ the consequence.

        Show
        stack added a comment - We need this? + public void setOperationTimeout(int operationTimeout) { Can't we just set it on construction and then be done w/ it rather than provide exotic options? This goes away? Not needed any more? Not even as a noop? private void beforeCall() { Patch looks great. +1. If we are removing some infinite timeout, good; commit and lets live w/ the consequence.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12630981/10606.v1.patch
        against trunk revision .
        ATTACHMENT ID: 12630981

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

        +1 tests included. The patch appears to include 3 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 mvn site goal succeeds with this patch.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//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/12630981/10606.v1.patch against trunk revision . ATTACHMENT ID: 12630981 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 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 mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8799//console This message is automatically generated.
        Hide
        Nicolas Liochon added a comment -

        v2 fixes the shared state for the call timeout and the remaining time. It becomes reusable (it is suppose to be reusable actually!). I remove the synchronized; could be risky. But it seems too strange to share this between threads.

        Show
        Nicolas Liochon added a comment - v2 fixes the shared state for the call timeout and the remaining time. It becomes reusable (it is suppose to be reusable actually!). I remove the synchronized; could be risky. But it seems too strange to share this between threads.
        Hide
        Nicolas Liochon added a comment -

        hum... This adds the timeouts in the scanner & the admin. I don't know if they are well configured... Before we were doing an infinite call whatever the scanner timeout. I wonder as well if' we're closing correctly the scanner server side in this case.

        Show
        Nicolas Liochon added a comment - hum... This adds the timeouts in the scanner & the admin. I don't know if they are well configured... Before we were doing an infinite call whatever the scanner timeout. I wonder as well if' we're closing correctly the scanner server side in this case.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development