HBase
  1. HBase
  2. HBASE-4741

Online schema change doesn't return errors

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 0.92.0
    • Fix Version/s: 0.92.0
    • Component/s: None
    • Hadoop Flags:
      Reviewed

      Description

      Still after the fun I had over in HBASE-4729, I tried to finish altering my table (remove a family) since only half of it was changed so I did this:

      hbase(main):002:0> alter 'TestTable', NAME => 'allo', METHOD => 'delete'
      Updating all regions with the new schema...
      244/244 regions updated.
      Done.
      0 row(s) in 1.2480 seconds

      Nice it all looks good, but over in the master log:

      org.apache.hadoop.hbase.InvalidFamilyOperationException: Family 'allo' does not exist so cannot be deleted
      at org.apache.hadoop.hbase.master.handler.TableDeleteFamilyHandler.handleTableOperation(TableDeleteFamilyHandler.java:56)
      at org.apache.hadoop.hbase.master.handler.TableEventHandler.process(TableEventHandler.java:86)
      at org.apache.hadoop.hbase.master.HMaster.deleteColumn(HMaster.java:1011)
      at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
      at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
      at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
      at java.lang.reflect.Method.invoke(Method.java:597)
      at org.apache.hadoop.hbase.ipc.WritableRpcEngine$Server.call(WritableRpcEngine.java:348)
      at org.apache.hadoop.hbase.ipc.HBaseServer$Handler.run(HBaseServer.java:1242)

      Maybe we should do checks before launching the async task.

      Marking critical as this is a regression.

      1. 4741.txt
        1 kB
        Ted Yu
      2. 4741-v2.txt
        2 kB
        Ted Yu
      3. 4741-v3.txt
        3 kB
        Ted Yu
      4. 4741-v4.txt
        3 kB
        Ted Yu
      5. 4741-v5.txt
        21 kB
        stack
      6. 4741-v6.txt
        21 kB
        stack
      7. 4741-v7.txt
        18 kB
        stack

        Activity

        Hide
        Ted Yu added a comment -

        If the additional check passes review, I will add such checks for other methods, such as modifyColumn().

        Show
        Ted Yu added a comment - If the additional check passes review, I will add such checks for other methods, such as modifyColumn().
        Hide
        Jean-Daniel Cryans added a comment -

        Instead of throwing an IOE a TableNotFoundException should be used.

        Show
        Jean-Daniel Cryans added a comment - Instead of throwing an IOE a TableNotFoundException should be used.
        Hide
        Jean-Daniel Cryans added a comment -

        Looks better, actually you could use checkTableModifiable, see how TableEventHandler uses it too. Also if you keep the same log message please fix the typo in "HTableDescritor".

        Show
        Jean-Daniel Cryans added a comment - Looks better, actually you could use checkTableModifiable, see how TableEventHandler uses it too. Also if you keep the same log message please fix the typo in "HTableDescritor".
        Hide
        Ted Yu added a comment -

        HBA uses this.connection.getMaster() to get to master.
        However there is no getter for MasterServices in HConnection interface.

        Show
        Ted Yu added a comment - HBA uses this.connection.getMaster() to get to master. However there is no getter for MasterServices in HConnection interface.
        Hide
        stack added a comment -

        However there is no getter for MasterServices in HConnection interface.

        Yeah... it'd be a bit odd having MasterServices in an HConnection that can be used in clients, regionservers and master.

        Show
        stack added a comment - However there is no getter for MasterServices in HConnection interface. Yeah... it'd be a bit odd having MasterServices in an HConnection that can be used in clients, regionservers and master.
        Hide
        Ted Yu added a comment -

        Added check for more operations.

        Show
        Ted Yu added a comment - Added check for more operations.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12502212/4741-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 -164 warning messages.

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

        -1 findbugs. The patch appears to introduce 46 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.master.TestDistributedLogSplitting
        org.apache.hadoop.hbase.client.TestAdmin
        org.apache.hadoop.hbase.coprocessor.TestMasterObserver
        org.apache.hadoop.hbase.master.TestMasterFailover

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/163//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/163//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/163//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/12502212/4741-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 -164 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 46 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.master.TestDistributedLogSplitting org.apache.hadoop.hbase.client.TestAdmin org.apache.hadoop.hbase.coprocessor.TestMasterObserver org.apache.hadoop.hbase.master.TestMasterFailover Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/163//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/163//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/163//console This message is automatically generated.
        Hide
        Jean-Daniel Cryans added a comment -

        HBA uses this.connection.getMaster() to get to master.

        Ha! I was somehow reading that you were doing the changes in HMaster, my bad. In this case checkTableExistence isn't needed because getTableDescriptor does what's needed (eg it will throw the TableNotFoundException).

        Reading the code I see stuff like:

          public void modifyColumn(final byte [] tableName, HColumnDescriptor descriptor)
          throws IOException {
            try {
              getMaster().modifyColumn(tableName, descriptor);
            } catch (RemoteException re) {
              // Convert RE exceptions in here; client shouldn't have to deal with them,
              // at least w/ the type of exceptions that come out of this method:
              // TableNotFoundException, etc.
              throw RemoteExceptionHandler.decodeRemoteException(re);
            }
          }
        
        

        So it seems that it used to be that the checks were done in the master. I would prefer to see the checks done over there since other clients (like asynchbase) would also need to implement the same checks.

        Back to the patch, it's also missing addColumn.

        Show
        Jean-Daniel Cryans added a comment - HBA uses this.connection.getMaster() to get to master. Ha! I was somehow reading that you were doing the changes in HMaster, my bad. In this case checkTableExistence isn't needed because getTableDescriptor does what's needed (eg it will throw the TableNotFoundException). Reading the code I see stuff like: public void modifyColumn( final byte [] tableName, HColumnDescriptor descriptor) throws IOException { try { getMaster().modifyColumn(tableName, descriptor); } catch (RemoteException re) { // Convert RE exceptions in here; client shouldn't have to deal with them, // at least w/ the type of exceptions that come out of this method: // TableNotFoundException, etc. throw RemoteExceptionHandler.decodeRemoteException(re); } } So it seems that it used to be that the checks were done in the master. I would prefer to see the checks done over there since other clients (like asynchbase) would also need to implement the same checks. Back to the patch, it's also missing addColumn.
        Hide
        Ted Yu added a comment -

        We already have this:

              getMaster().deleteColumn(tableName, columnName);
            } catch (RemoteException e) {
              throw RemoteExceptionHandler.decodeRemoteException(e);
            }
        

        I wonder why exception decoding didn't work in the first place.

        Show
        Ted Yu added a comment - We already have this: getMaster().deleteColumn(tableName, columnName); } catch (RemoteException e) { throw RemoteExceptionHandler.decodeRemoteException(e); } I wonder why exception decoding didn't work in the first place.
        Hide
        Ted Yu added a comment -

        Since the check is made synchronously now, some tests failed, e.g.
        https://builds.apache.org/job/PreCommit-HBASE-Build/163/testReport/org.apache.hadoop.hbase.coprocessor/TestMasterObserver/testTableOperations/
        where the following assertion previously worked because cpHost.preModifyColumn() was called prior to any such checking:

             assertTrue("Second column family should be modified",
               cp.preModifyColumnCalledOnly());
        

        We should define new strategy accordingly.

        Show
        Ted Yu added a comment - Since the check is made synchronously now, some tests failed, e.g. https://builds.apache.org/job/PreCommit-HBASE-Build/163/testReport/org.apache.hadoop.hbase.coprocessor/TestMasterObserver/testTableOperations/ where the following assertion previously worked because cpHost.preModifyColumn() was called prior to any such checking: assertTrue( "Second column family should be modified" , cp.preModifyColumnCalledOnly()); We should define new strategy accordingly.
        Hide
        Ted Yu added a comment -

        Patch v4 removes an unnecessary deleteColumn() call.

        Show
        Ted Yu added a comment - Patch v4 removes an unnecessary deleteColumn() call.
        Hide
        Ted Yu added a comment -

        I may not have time to work on this in the next week.

        Show
        Ted Yu added a comment - I may not have time to work on this in the next week.
        Hide
        Hadoop QA added a comment -

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

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

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

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

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

        -1 findbugs. The patch appears to introduce 46 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.thrift2.TestThriftHBaseServiceHandler
        org.apache.hadoop.hbase.client.TestAdmin
        org.apache.hadoop.hbase.coprocessor.TestMasterObserver
        org.apache.hadoop.hbase.master.TestDistributedLogSplitting
        org.apache.hadoop.hbase.master.TestMasterFailover

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/172//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/172//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/172//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/12502464/4741-v4.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -164 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 46 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.thrift2.TestThriftHBaseServiceHandler org.apache.hadoop.hbase.client.TestAdmin org.apache.hadoop.hbase.coprocessor.TestMasterObserver org.apache.hadoop.hbase.master.TestDistributedLogSplitting org.apache.hadoop.hbase.master.TestMasterFailover Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/172//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/172//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/172//console This message is automatically generated.
        Hide
        stack added a comment -

        Here is a version to do the testing server-side up in the TableEventHandler constructors before the Excecutor is submitted so the error can go back to the user.

        This is a bit of a regression for sure. This stuff use to be all nice working but I think refactor as executors seems to have lost these nice little errors.

        I've added a test that has a good few of these conditions listed out.

        There are some issues in TestAdmin that I'm looking at but will run patch build to see if I've broken anything else.

        Show
        stack added a comment - Here is a version to do the testing server-side up in the TableEventHandler constructors before the Excecutor is submitted so the error can go back to the user. This is a bit of a regression for sure. This stuff use to be all nice working but I think refactor as executors seems to have lost these nice little errors. I've added a test that has a good few of these conditions listed out. There are some issues in TestAdmin that I'm looking at but will run patch build to see if I've broken anything else.
        Hide
        Hadoop QA added a comment -

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

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

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

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/223//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/12503180/4741-v5.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/223//console This message is automatically generated.
        Hide
        stack added a comment -

        Fix TestAdmin. Fix import that was causing this patch to apply.

        Show
        stack added a comment - Fix TestAdmin. Fix import that was causing this patch to apply.
        Hide
        stack added a comment -

        Any chance of a review here?

        Show
        stack added a comment - Any chance of a review here?
        Hide
        Lars Hofhansl added a comment -

        +1 of v6
        The changes in HMaster.java are no-ops, right?

        Show
        Lars Hofhansl added a comment - +1 of v6 The changes in HMaster.java are no-ops, right?
        Hide
        stack added a comment -

        @Lars Yes. Let me leave those out the HMaster changes. They are not necessary. Thanks for the review.

        Show
        stack added a comment - @Lars Yes. Let me leave those out the HMaster changes. They are not necessary. Thanks for the review.
        Hide
        stack added a comment -

        v7 is v6 minus HMaster changes.

        Show
        stack added a comment - v7 is v6 minus HMaster changes.
        Hide
        stack added a comment -

        Committed branch and trunk.

        Show
        stack added a comment - Committed branch and trunk.
        Hide
        Hadoop QA added a comment -

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

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

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

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

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

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

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

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

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/229//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/229//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/229//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/12503307/4741-v6.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -164 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 49 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/229//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/229//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/229//console This message is automatically generated.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2427 (See https://builds.apache.org/job/HBase-TRUNK/2427/)
        HBASE-4741 Online schema change doesn't return errors

        stack :
        Files :

        • /hbase/trunk/CHANGES.txt
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ModifyTableHandler.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/TableAddFamilyHandler.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/TableDeleteFamilyHandler.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/TableModifyFamilyHandler.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2427 (See https://builds.apache.org/job/HBase-TRUNK/2427/ ) HBASE-4741 Online schema change doesn't return errors stack : Files : /hbase/trunk/CHANGES.txt /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ModifyTableHandler.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/TableAddFamilyHandler.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/TableDeleteFamilyHandler.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/TableModifyFamilyHandler.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.92 #123 (See https://builds.apache.org/job/HBase-0.92/123/)
        HBASE-4741 Online schema change doesn't return errors

        stack :
        Files :

        • /hbase/branches/0.92/CHANGES.txt
        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java
        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java
        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/handler/ModifyTableHandler.java
        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/handler/TableAddFamilyHandler.java
        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/handler/TableDeleteFamilyHandler.java
        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java
        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/handler/TableModifyFamilyHandler.java
        • /hbase/branches/0.92/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java
        Show
        Hudson added a comment - Integrated in HBase-0.92 #123 (See https://builds.apache.org/job/HBase-0.92/123/ ) HBASE-4741 Online schema change doesn't return errors stack : Files : /hbase/branches/0.92/CHANGES.txt /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/handler/ModifyTableHandler.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/handler/TableAddFamilyHandler.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/handler/TableDeleteFamilyHandler.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/master/handler/TableModifyFamilyHandler.java /hbase/branches/0.92/src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java

          People

          • Assignee:
            stack
            Reporter:
            Jean-Daniel Cryans
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development