Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.94.0
    • Fix Version/s: 0.94.0
    • Component/s: Client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      What HBASE-4805 does for HTables, this should do for HBaseAdmin.
      Along with this the shared error handling and retrying between HBaseAdmin and HConnectionManager can also be improved. I'll attach a first pass patch soon.

      1. 5058-v3.txt
        4 kB
        Lars Hofhansl
      2. 5058-v3.txt
        4 kB
        stack
      3. 5058-v2.txt
        4 kB
        Lars Hofhansl
      4. 5058.txt
        4 kB
        Lars Hofhansl

        Issue Links

          Activity

          Hide
          Lars Hofhansl added a comment -

          This patch adds an option to use an externally managed HConnection, and also simplifies the retry logic in HBaseAdmin.

          This is a small change, but please review carefully.

          Show
          Lars Hofhansl added a comment - This patch adds an option to use an externally managed HConnection, and also simplifies the retry logic in HBaseAdmin. This is a small change, but please review carefully.
          Hide
          Lars Hofhansl added a comment -

          Test run

          Show
          Lars Hofhansl added a comment - Test run
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12507730/5058.txt
          against trunk revision .

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          -1 javadoc. The javadoc tool appears to have generated -152 warning messages.

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

          -1 findbugs. The patch appears to introduce 76 new Findbugs (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.replication.TestReplication
          org.apache.hadoop.hbase.master.TestMasterRestartAfterDisablingTable

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/526//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/526//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/526//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/12507730/5058.txt against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 javadoc. The javadoc tool appears to have generated -152 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 76 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.replication.TestReplication org.apache.hadoop.hbase.master.TestMasterRestartAfterDisablingTable Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/526//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/526//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/526//console This message is automatically generated.
          Hide
          Lars Hofhansl added a comment -

          Both TestMasterRestartAfterDisablingTable and TestReplication pass locally.

          Show
          Lars Hofhansl added a comment - Both TestMasterRestartAfterDisablingTable and TestReplication pass locally.
          Hide
          stack added a comment -

          Is this check too generic:

          +        } catch (UndeclaredThrowableException ute) {
          

          For sure if above, you can deduce:

          +          // RPC connection to the master is dead, try again
          

          Will this change behavior? Or is it that what we have here is dumb?

                 } catch (MasterNotRunningException mnre) {
          -        HConnectionManager.deleteStaleConnection(this.connection);
          -        this.connection = HConnectionManager.getConnection(this.conf);
          -      } catch (UndeclaredThrowableException ute) {
          -        HConnectionManager.deleteStaleConnection(this.connection);
          -        this.connection = HConnectionManager.getConnection(this.conf);
          +        // ignore and try again
          

          If making new patch, fix this javadoc... missing 'in'

          +   * The HConnection can be re-used again another attempt.
          

          Otherwise, looks good.

          Show
          stack added a comment - Is this check too generic: + } catch (UndeclaredThrowableException ute) { For sure if above, you can deduce: + // RPC connection to the master is dead, try again Will this change behavior? Or is it that what we have here is dumb? } catch (MasterNotRunningException mnre) { - HConnectionManager.deleteStaleConnection( this .connection); - this .connection = HConnectionManager.getConnection( this .conf); - } catch (UndeclaredThrowableException ute) { - HConnectionManager.deleteStaleConnection( this .connection); - this .connection = HConnectionManager.getConnection( this .conf); + // ignore and try again If making new patch, fix this javadoc... missing 'in' + * The HConnection can be re-used again another attempt. Otherwise, looks good.
          Hide
          Lars Hofhansl added a comment -

          Thanks for reviewing stack.

          Re UndeclaredThrowableException. HBaseAdmin used to catch it, but it should have never leaked up to HBaseAdmin. According to the documentation it is "Thrown by a method invocation on a proxy instance...". I verified experimentally that this exception is indeed thrown when I shutdown the cluster and try to reuse the HConnection (or before the HBaseAdmin) instance. Maybe that is another bug.

          The 2nd part, yeah I think that was just dumb before. There are two levels of retrying. HConnection already has a retry mechanism. That said, it seems that for unmanaged connections (i.e. created by HBaseAdmin itself) the HConnectionManager.HConnectionImplementation.masterChecked would not reset to false. I was thinking about just removing that flag, and that in fact might be a better option. Then every call to HConnection.getMaster would try again, which is as it should be. Let me make a new patch with this.

          Show
          Lars Hofhansl added a comment - Thanks for reviewing stack. Re UndeclaredThrowableException. HBaseAdmin used to catch it, but it should have never leaked up to HBaseAdmin. According to the documentation it is "Thrown by a method invocation on a proxy instance...". I verified experimentally that this exception is indeed thrown when I shutdown the cluster and try to reuse the HConnection (or before the HBaseAdmin) instance. Maybe that is another bug. The 2nd part, yeah I think that was just dumb before. There are two levels of retrying. HConnection already has a retry mechanism. That said, it seems that for unmanaged connections (i.e. created by HBaseAdmin itself) the HConnectionManager.HConnectionImplementation.masterChecked would not reset to false. I was thinking about just removing that flag, and that in fact might be a better option. Then every call to HConnection.getMaster would try again, which is as it should be. Let me make a new patch with this.
          Hide
          Lars Hofhansl added a comment -

          Seems I need to think about this a bit more. There are subtle threading issues here (the HConnectionImplementation's master can now suddenly be set to null).
          What I need to ensure that is an unmanaged HConnection ever gets into a state where it just gives up and never retries in the expectation that it will be closed and re-created anyway.

          Show
          Lars Hofhansl added a comment - Seems I need to think about this a bit more. There are subtle threading issues here (the HConnectionImplementation's master can now suddenly be set to null). What I need to ensure that is an unmanaged HConnection ever gets into a state where it just gives up and never retries in the expectation that it will be closed and re-created anyway.
          Hide
          Lars Hofhansl added a comment -

          Marking this as invalid. HBaseAdmin is not used nearly as often as HTable, and a bit churn through HConnction is not so bad.

          There are layers of retrying (which is bad) and RPC runtime exceptions are passed to up to HBaseAdmin (also bad). But none of those are horrible.

          The entire client needs to be revisited, this is not the jira to do that.

          Show
          Lars Hofhansl added a comment - Marking this as invalid. HBaseAdmin is not used nearly as often as HTable, and a bit churn through HConnction is not so bad. There are layers of retrying (which is bad) and RPC runtime exceptions are passed to up to HBaseAdmin (also bad). But none of those are horrible. The entire client needs to be revisited, this is not the jira to do that.
          Hide
          Lars Hofhansl added a comment -

          Actually, never mind. I have a simple patch now, that bandaids the problem.

          Show
          Lars Hofhansl added a comment - Actually, never mind. I have a simple patch now, that bandaids the problem.
          Hide
          Lars Hofhansl added a comment -

          Simpler patch. Just avoid throwing RPC related runtime exceptions up to the client along with a simple new constructor for HBaseAdmin.

          Show
          Lars Hofhansl added a comment - Simpler patch. Just avoid throwing RPC related runtime exceptions up to the client along with a simple new constructor for HBaseAdmin.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12507817/5058-v2.txt
          against trunk revision .

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          -1 javadoc. The javadoc tool appears to have generated -152 warning messages.

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

          -1 findbugs. The patch appears to introduce 76 new Findbugs (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.regionserver.wal.TestLogRolling
          org.apache.hadoop.hbase.client.TestInstantSchemaChange

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/535//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/535//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/535//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/12507817/5058-v2.txt against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 javadoc. The javadoc tool appears to have generated -152 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 76 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.regionserver.wal.TestLogRolling org.apache.hadoop.hbase.client.TestInstantSchemaChange Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/535//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/535//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/535//console This message is automatically generated.
          Hide
          Lars Hofhansl added a comment -

          @stack: are you OK with this patch?

          Show
          Lars Hofhansl added a comment - @stack: are you OK with this patch?
          Hide
          Lars Hofhansl added a comment -

          Another option is to unwrap the UndeclaredThrowableException and rethrow the IOException (if any) that caused it. getMaster would then declare IOException rather than ZooKeeperException in its throws clause. That might be the cleanest approach.

          Show
          Lars Hofhansl added a comment - Another option is to unwrap the UndeclaredThrowableException and rethrow the IOException (if any) that caused it. getMaster would then declare IOException rather than ZooKeeperException in its throws clause. That might be the cleanest approach.
          Hide
          stack added a comment -

          +1

          Show
          stack added a comment - +1
          Hide
          Lars Hofhansl added a comment -

          +1 for the patch, or the latest comment?

          (I did explore a patch for the last comment, but it turns out that this will either have API implications - user code now needs to deal with IOException, or it needs a lot of changes to map IOException back other exceptions.)

          Show
          Lars Hofhansl added a comment - +1 for the patch, or the latest comment? (I did explore a patch for the last comment, but it turns out that this will either have API implications - user code now needs to deal with IOException, or it needs a lot of changes to map IOException back other exceptions.)
          Hide
          Ted Yu added a comment -

          Since this JIRA is targeting TRUNK, I think change of API should be allowed if we agree it is on the right track.

          Show
          Ted Yu added a comment - Since this JIRA is targeting TRUNK, I think change of API should be allowed if we agree it is on the right track.
          Hide
          Lars Hofhansl added a comment -

          Hmm... Good point. Although I now think my current patch is the safest. I think I would just wrap UndeclaredThrowableException (rather than RuntimeException) into a MasterNotRunning exception, but that's it.
          There's also the question what happens when the master moves (2nd master takes over for example). Since the HConnectionImplementation caches the HMasterInterface, it will probably never switch.

          In general this code is a bit messy, which is why I am flip-flopping here... No solution seems quite right.

          Show
          Lars Hofhansl added a comment - Hmm... Good point. Although I now think my current patch is the safest. I think I would just wrap UndeclaredThrowableException (rather than RuntimeException) into a MasterNotRunning exception, but that's it. There's also the question what happens when the master moves (2nd master takes over for example). Since the HConnectionImplementation caches the HMasterInterface, it will probably never switch. In general this code is a bit messy, which is why I am flip-flopping here... No solution seems quite right.
          Hide
          Ted Yu added a comment -
          +        // If didn't get the master and this is a managed connection, give up.
          +        // Otherwise give subsequent calls a chance to try again.
          +        this.masterChecked = managed || master != null;
          

          Judging from the first sentence of the javadoc above, the assignment should be

          +        this.masterChecked = !managed || master != null;
          

          Basically the negation of RHS becomes: managed && master == null

          Show
          Ted Yu added a comment - + // If didn't get the master and this is a managed connection, give up. + // Otherwise give subsequent calls a chance to try again. + this .masterChecked = managed || master != null ; Judging from the first sentence of the javadoc above, the assignment should be + this .masterChecked = !managed || master != null ; Basically the negation of RHS becomes: managed && master == null
          Hide
          Lars Hofhansl added a comment -

          Actually it is the other way round...

          masterChecked is set to true to avoid trying to retrieve the master in the future. This is fine for the managed HConnection as it will just be removed and another is created when needed.
          For an HConnection that is passed from the outside, it has to be possible to try again. So if the HConnection is managed we retain the old behavior (i.e. only try once, give up after that, even if that failed).
          For an unmanaged connection we try again unless we actually found a master. So masterChecked is set to true if either the connection is managed (always avoid retrying), or we found a master.

          Show
          Lars Hofhansl added a comment - Actually it is the other way round... masterChecked is set to true to avoid trying to retrieve the master in the future. This is fine for the managed HConnection as it will just be removed and another is created when needed. For an HConnection that is passed from the outside, it has to be possible to try again. So if the HConnection is managed we retain the old behavior (i.e. only try once, give up after that, even if that failed). For an unmanaged connection we try again unless we actually found a master. So masterChecked is set to true if either the connection is managed (always avoid retrying), or we found a master.
          Hide
          Ted Yu added a comment -

          The above description is clearer than the javadoc in the patch (esp. 'give up').

          Show
          Ted Yu added a comment - The above description is clearer than the javadoc in the patch (esp. 'give up').
          Hide
          Lars Hofhansl added a comment -

          Here's an attempt I can live with.
          Notice how (1) the master null check is pulled into the synchronized block and (2) master is now set to null before the start of the loop (3) sets masterChecked to managed.

          The effect is that the current behavior is not changed. I.e. for a managed connection we try only once. Unmanaged connection get a chance to retry on subsequent calls, and since master is set to null, this would work even when the master has moved since the first attempt.

          Show
          Lars Hofhansl added a comment - Here's an attempt I can live with. Notice how (1) the master null check is pulled into the synchronized block and (2) master is now set to null before the start of the loop (3) sets masterChecked to managed. The effect is that the current behavior is not changed. I.e. for a managed connection we try only once. Unmanaged connection get a chance to retry on subsequent calls, and since master is set to null, this would work even when the master has moved since the first attempt.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12507875/5058-v3.txt
          against trunk revision .

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          -1 javadoc. The javadoc tool appears to have generated -152 warning messages.

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

          -1 findbugs. The patch appears to introduce 76 new Findbugs (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these unit tests:

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/541//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/541//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/541//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/12507875/5058-v3.txt against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 javadoc. The javadoc tool appears to have generated -152 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 76 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/541//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/541//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/541//console This message is automatically generated.
          Hide
          Lars Hofhansl added a comment -

          Anybody opposed to the last patch? This is what I would like to commit to trunk...

          Show
          Lars Hofhansl added a comment - Anybody opposed to the last patch? This is what I would like to commit to trunk...
          Hide
          Ted Yu added a comment -

          Hadoop QA didn't finish test suite:

          Running org.apache.hadoop.hbase.replication.TestReplicationSource
          Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.617 sec
          
          Results :
          
          Tests run: 8, Failures: 0, Errors: 0, Skipped: 0
          
          [INFO] ------------------------------------------------------------------------
          [INFO] BUILD FAILURE
          [INFO] -------------------------------------------------------------
          
          Show
          Ted Yu added a comment - Hadoop QA didn't finish test suite: Running org.apache.hadoop.hbase.replication.TestReplicationSource Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.617 sec Results : Tests run: 8, Failures: 0, Errors: 0, Skipped: 0 [INFO] ------------------------------------------------------------------------ [INFO] BUILD FAILURE [INFO] -------------------------------------------------------------
          Hide
          Ted Yu added a comment -
          +        LOG.info("Exception contacting master. Retrying...", ute.getCause());
          

          Would WARN be better log level ?

          -        throw new MasterNotRunningException(sn.toString());
          +        return this.master;
          

          Looks like assignment to this.masterChecked is missing before returning (for unmanaged case).

          Show
          Ted Yu added a comment - + LOG.info( "Exception contacting master. Retrying..." , ute.getCause()); Would WARN be better log level ? - throw new MasterNotRunningException(sn.toString()); + return this .master; Looks like assignment to this.masterChecked is missing before returning (for unmanaged case).
          Hide
          stack added a comment -

          I was +1'ing the patch. Still +1 (you think it caused hang above?)

          Show
          stack added a comment - I was +1'ing the patch. Still +1 (you think it caused hang above?)
          Hide
          Lars Hofhansl added a comment -

          @stack: I doubt this caused the hang, but I'll investigate.
          @Ted: The patch just looks weird, the assignment to this.master and the return was pulled into the synchronized statement (because it is now set to null above to allow for retrying).

          Show
          Lars Hofhansl added a comment - @stack: I doubt this caused the hang, but I'll investigate. @Ted: The patch just looks weird, the assignment to this.master and the return was pulled into the synchronized statement (because it is now set to null above to allow for retrying).
          Hide
          Lars Hofhansl added a comment -

          @Ted: I had LOG.info here, because for all other cases below we also log at the info level. The master can come and go, and the clients should still work, so info seems right. Happy to change it to warn if you think that makes more sense.

          Show
          Lars Hofhansl added a comment - @Ted: I had LOG.info here, because for all other cases below we also log at the info level. The master can come and go, and the clients should still work, so info seems right. Happy to change it to warn if you think that makes more sense.
          Hide
          Lars Hofhansl added a comment -

          Getting another test run

          Show
          Lars Hofhansl added a comment - Getting another test run
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12507950/5058-v3.txt
          against trunk revision .

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          -1 javadoc. The javadoc tool appears to have generated -152 warning messages.

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

          -1 findbugs. The patch appears to introduce 76 new Findbugs (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these unit tests:

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/548//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/548//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/548//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/12507950/5058-v3.txt against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 javadoc. The javadoc tool appears to have generated -152 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 76 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/548//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/548//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/548//console This message is automatically generated.
          Hide
          stack added a comment -

          My fault. Compilation error now fixed.

          Show
          stack added a comment - My fault. Compilation error now fixed.
          Hide
          stack added a comment -

          Resubmit.

          Show
          stack added a comment - Resubmit.
          Hide
          Lars Hofhansl added a comment -

          Thanks Stack!

          Show
          Lars Hofhansl added a comment - Thanks Stack!
          Hide
          Ted Yu added a comment -

          Log level @ INFO is fine.

          Show
          Ted Yu added a comment - Log level @ INFO is fine.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12507956/5058-v3.txt
          against trunk revision .

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          -1 javadoc. The javadoc tool appears to have generated -152 warning messages.

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

          -1 findbugs. The patch appears to introduce 76 new Findbugs (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.client.TestInstantSchemaChange
          org.apache.hadoop.hbase.mapred.TestTableMapReduce
          org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/549//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/549//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/549//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/12507956/5058-v3.txt against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 javadoc. The javadoc tool appears to have generated -152 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 76 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.client.TestInstantSchemaChange org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/549//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/549//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/549//console This message is automatically generated.
          Hide
          Lars Hofhansl added a comment -

          Ran the three failing tests locally with the patch... They all pass fine.

          @Ted: You OK with the change?

          Show
          Lars Hofhansl added a comment - Ran the three failing tests locally with the patch... They all pass fine. @Ted: You OK with the change?
          Hide
          Ted Yu added a comment -

          I am fine with the change.

          Show
          Ted Yu added a comment - I am fine with the change.
          Hide
          Lars Hofhansl added a comment -

          Committed to trunk. Also added a very simple test as part of the commit.

          Show
          Lars Hofhansl added a comment - Committed to trunk. Also added a very simple test as part of the commit.
          Hide
          Lars Hofhansl added a comment -

          Thanks for the review Stack and Ted.
          Is it time to rewrite the client, yet?

          Show
          Lars Hofhansl added a comment - Thanks for the review Stack and Ted. Is it time to rewrite the client, yet?
          Hide
          stack added a comment -

          Its time. Pull in asynchbase?

          Show
          stack added a comment - Its time. Pull in asynchbase?
          Hide
          Ted Yu added a comment -

          Pull in asynchbase?

          Why not.

          Show
          Ted Yu added a comment - Pull in asynchbase? Why not.
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2561 (See https://builds.apache.org/job/HBase-TRUNK/2561/)
          HBASE-5058 Allow HBaseAmin to use an existing connection (Lars H)

          larsh :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2561 (See https://builds.apache.org/job/HBase-TRUNK/2561/ ) HBASE-5058 Allow HBaseAmin to use an existing connection (Lars H) larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
          Hide
          Lars Hofhansl added a comment -

          Re: asynchhbase. I could start by creating a jira
          Seriously, we'd have change some things, add missing features, and then maintain it going forward.

          Show
          Lars Hofhansl added a comment - Re: asynchhbase. I could start by creating a jira Seriously, we'd have change some things, add missing features, and then maintain it going forward.
          Hide
          stack added a comment -

          Whats the current state of the case against our current client?

          + The Jonathan Payne accounting of unaccounted off-heap socket buffers per thread which makes our client OOME when lots of threads
          + Messy timeout story
          + "complex", lots of layers, long-lived zk connection (not necessary on client?)

          What else?

          Show
          stack added a comment - Whats the current state of the case against our current client? + The Jonathan Payne accounting of unaccounted off-heap socket buffers per thread which makes our client OOME when lots of threads + Messy timeout story + "complex", lots of layers, long-lived zk connection (not necessary on client?) What else?
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-security #38 (See https://builds.apache.org/job/HBase-TRUNK-security/38/)
          HBASE-5058 Allow HBaseAmin to use an existing connection (Lars H)

          larsh :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-security #38 (See https://builds.apache.org/job/HBase-TRUNK-security/38/ ) HBASE-5058 Allow HBaseAmin to use an existing connection (Lars H) larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
          Hide
          Lars Hofhansl added a comment - - edited

          @Stack: I think that about sums it up. The complexity of layers and timeout stories are alleviated somewhat by parent HBASE-4805 (no per HTable threadpool, HTablePool no longer needed).
          I had a brief look at the first issue, unless I am missing something this would require a nontrivial amount of refactoring. The simplest would be to do all network IO from the Connection thread rather than the application thread (as described in HBASE-4956). Would need allow for the client to synchronize and retrieve exceptions on/from a Future.

          Short term, should we take HBASE-4805 all the way and add a getTable(...) method to HConnection? (Or even further and add put/get/scan/etc methods that take a table name to HConnection?)

          Long term a design based on asynchhbase with a thin synchronous layer on top is probably the best option.

          Show
          Lars Hofhansl added a comment - - edited @Stack: I think that about sums it up. The complexity of layers and timeout stories are alleviated somewhat by parent HBASE-4805 (no per HTable threadpool, HTablePool no longer needed). I had a brief look at the first issue, unless I am missing something this would require a nontrivial amount of refactoring. The simplest would be to do all network IO from the Connection thread rather than the application thread (as described in HBASE-4956 ). Would need allow for the client to synchronize and retrieve exceptions on/from a Future. Short term, should we take HBASE-4805 all the way and add a getTable(...) method to HConnection? (Or even further and add put/get/scan/etc methods that take a table name to HConnection?) Long term a design based on asynchhbase with a thin synchronous layer on top is probably the best option.
          Hide
          stack added a comment -

          @Lars We already have an issue, hbase-3523 "Rewrite our client (client 2.0)"

          Show
          stack added a comment - @Lars We already have an issue, hbase-3523 "Rewrite our client (client 2.0)"
          Hide
          Lars Hofhansl added a comment -

          Ah thanks... I cut and pasted the same into the SVN commit log

          Show
          Lars Hofhansl added a comment - Ah thanks... I cut and pasted the same into the SVN commit log

            People

            • Assignee:
              Lars Hofhansl
              Reporter:
              Lars Hofhansl
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development