HBASE-4741 Online schema change doesn't return errors

git-svn-id: https://svn.apache.org/repos/asf/hbase/trunk@1200660 13f79535-47bb-0310-9956-ffa450edef68

CHANGES.txt

@@ -464,6 +464,7 @@ Release 0.92.0 - Unreleased
    HBASE-4754  FSTableDescriptors.getTableInfoPath() should handle FileNotFoundException
    HBASE-4740  [bulk load] the HBASE-4552 API can't tell if errors on region server are recoverable
                (Jonathan Hsieh)
+   HBASE-4741  Online schema change doesn't return errors
 
   TESTS
    HBASE-4450  test for number of blocks read: to serve as baseline for expected

src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java

@@ -129,7 +129,7 @@ public class HBaseAdmin implements Abortable, Closeable {
    * to cleanup the returned catalog tracker.
    * @throws ZooKeeperConnectionException
    * @throws IOException
-   * @see #cleanupCatalogTracker(CatalogTracker);
+   * @see #cleanupCatalogTracker(CatalogTracker)
    */
   private synchronized CatalogTracker getCatalogTracker()
   throws ZooKeeperConnectionException, IOException {

src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java

@@ -24,14 +24,9 @@ import java.util.List;
 
 import org.apache.hadoop.hbase.ClusterStatus;
 import org.apache.hadoop.hbase.HColumnDescriptor;
-import org.apache.hadoop.hbase.HRegionInfo;
 import org.apache.hadoop.hbase.HTableDescriptor;
 import org.apache.hadoop.hbase.UnknownRegionException;
 import org.apache.hadoop.hbase.util.Pair;
-import org.apache.hadoop.hbase.coprocessor.ObserverContext;
-import org.apache.hadoop.hbase.ipc.VersionedProtocol;
-
-
 
 /**
  * Clients interact with the HMasterInterface to gain access to meta-level

src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java

@@ -40,6 +40,8 @@ public class DeleteTableHandler extends TableEventHandler {
       final MasterServices masterServices)
   throws IOException {
     super(EventType.C_M_DELETE_TABLE, tableName, server, masterServices);
+    // The next call fails if no such table.
+    getTableDescriptor();
   }
 
   @Override

src/main/java/org/apache/hadoop/hbase/master/handler/ModifyTableHandler.java

@@ -26,20 +26,19 @@ import org.apache.hadoop.hbase.HRegionInfo;
 import org.apache.hadoop.hbase.HTableDescriptor;
 import org.apache.hadoop.hbase.Server;
 import org.apache.hadoop.hbase.master.MasterServices;
-import org.apache.hadoop.hbase.util.Bytes;
 
 public class ModifyTableHandler extends TableEventHandler {
   private final HTableDescriptor htd;
 
   public ModifyTableHandler(final byte [] tableName,
       final HTableDescriptor htd, final Server server,
-      final MasterServices masterServices) throws IOException {
+      final MasterServices masterServices)
+  throws IOException {
     super(EventType.C_M_MODIFY_TABLE, tableName, server, masterServices);
+    // Check table exists.
+    getTableDescriptor();
+    // This is the new schema we are going to write out as this modification.
     this.htd = htd;
-    if (!Bytes.equals(tableName, htd.getName())) {
-      throw new IOException("TableDescriptor name & tableName must match: " 
-          + htd.getNameAsString() + " vs " + Bytes.toString(tableName));
-    }
   }
 
   @Override
@@ -55,6 +54,7 @@ public class ModifyTableHandler extends TableEventHandler {
     if(server != null && server.getServerName() != null) {
       name = server.getServerName().toString();
     }
-    return getClass().getSimpleName() + "-" + name + "-" + getSeqid() + "-" + tableNameStr;
+    return getClass().getSimpleName() + "-" + name + "-" + getSeqid() + "-" +
+      tableNameStr;
   }
 }

src/main/java/org/apache/hadoop/hbase/master/handler/TableAddFamilyHandler.java

@@ -28,7 +28,6 @@ import org.apache.hadoop.hbase.HTableDescriptor;
 import org.apache.hadoop.hbase.InvalidFamilyOperationException;
 import org.apache.hadoop.hbase.Server;
 import org.apache.hadoop.hbase.master.MasterServices;
-import org.apache.hadoop.hbase.util.Bytes;
 
 /**
  * Handles adding a new family to an existing table.
@@ -40,27 +39,19 @@ public class TableAddFamilyHandler extends TableEventHandler {
   public TableAddFamilyHandler(byte[] tableName, HColumnDescriptor familyDesc,
       Server server, final MasterServices masterServices) throws IOException {
     super(EventType.C_M_ADD_FAMILY, tableName, server, masterServices);
+    HTableDescriptor htd = getTableDescriptor();
+    if (htd.hasFamily(familyDesc.getName())) {
+      throw new InvalidFamilyOperationException("Family '" +
+        familyDesc.getNameAsString() + "' already exists so cannot be added");
+    }
     this.familyDesc = familyDesc;
   }
 
   @Override
   protected void handleTableOperation(List<HRegionInfo> hris)
   throws IOException {
-    HTableDescriptor htd = this.masterServices.getTableDescriptors().
-                                      get(Bytes.toString(tableName));
-    byte [] familyName = familyDesc.getName();
-    if (htd == null) {
-      throw new IOException("Add Family operation could not be completed as " +
-          "HTableDescritor is missing for table = "
-          + Bytes.toString(tableName));
-    }
-    if(htd.hasFamily(familyName)) {
-      throw new InvalidFamilyOperationException(
-          "Family '" + Bytes.toString(familyName) + "' already exists so " +
-          "cannot be added");
-    }
     // Update table descriptor in HDFS
-    htd = this.masterServices.getMasterFileSystem()
+    HTableDescriptor htd = this.masterServices.getMasterFileSystem()
         .addColumn(tableName, familyDesc);
     // Update in-memory descriptor cache
     this.masterServices.getTableDescriptors().add(htd);

src/main/java/org/apache/hadoop/hbase/master/handler/TableDeleteFamilyHandler.java

@@ -24,9 +24,7 @@ import java.util.List;
 
 import org.apache.hadoop.hbase.HRegionInfo;
 import org.apache.hadoop.hbase.HTableDescriptor;
-import org.apache.hadoop.hbase.InvalidFamilyOperationException;
 import org.apache.hadoop.hbase.Server;
-import org.apache.hadoop.hbase.master.AssignmentManager;
 import org.apache.hadoop.hbase.master.MasterServices;
 import org.apache.hadoop.hbase.util.Bytes;
 
@@ -40,26 +38,15 @@ public class TableDeleteFamilyHandler extends TableEventHandler {
   public TableDeleteFamilyHandler(byte[] tableName, byte [] familyName,
       Server server, final MasterServices masterServices) throws IOException {
     super(EventType.C_M_ADD_FAMILY, tableName, server, masterServices);
-    this.familyName = familyName;
+    HTableDescriptor htd = getTableDescriptor();
+    this.familyName = hasColumnFamily(htd, familyName);
   }
 
   @Override
   protected void handleTableOperation(List<HRegionInfo> hris) throws IOException {
-    AssignmentManager am = this.masterServices.getAssignmentManager();
-    HTableDescriptor htd = this.masterServices.getTableDescriptors().get(Bytes.toString(tableName));
-    if (htd == null) {
-      throw new IOException("Add Family operation could not be completed as " +
-          "HTableDescritor is missing for table = "
-          + Bytes.toString(tableName));
-    }
-    if(!htd.hasFamily(familyName)) {
-      throw new InvalidFamilyOperationException(
-          "Family '" + Bytes.toString(familyName) + "' does not exist so " +
-          "cannot be deleted");
-    }
     // Update table descriptor in HDFS
-    htd = this.masterServices.getMasterFileSystem()
-        .deleteColumn(tableName, familyName);
+    HTableDescriptor htd =
+      this.masterServices.getMasterFileSystem().deleteColumn(tableName, familyName);
     // Update in-memory descriptor cache
     this.masterServices.getTableDescriptors().add(htd);
   }

src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java

@@ -19,6 +19,7 @@
  */
 package org.apache.hadoop.hbase.master.handler;
 
+import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.LinkedList;
@@ -29,8 +30,11 @@ import java.util.TreeMap;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.apache.hadoop.hbase.HRegionInfo;
+import org.apache.hadoop.hbase.HTableDescriptor;
+import org.apache.hadoop.hbase.InvalidFamilyOperationException;
 import org.apache.hadoop.hbase.Server;
 import org.apache.hadoop.hbase.ServerName;
+import org.apache.hadoop.hbase.TableExistsException;
 import org.apache.hadoop.hbase.TableNotDisabledException;
 import org.apache.hadoop.hbase.catalog.MetaReader;
 import org.apache.hadoop.hbase.client.HTable;
@@ -148,6 +152,33 @@ public abstract class TableEventHandler extends EventHandler {
     }
     return done;
   }
+
+  /**
+   * @return Table descriptor for this table
+   * @throws TableExistsException
+   * @throws FileNotFoundException
+   * @throws IOException
+   */
+  HTableDescriptor getTableDescriptor()
+  throws TableExistsException, FileNotFoundException, IOException {
+    final String name = Bytes.toString(tableName);
+    HTableDescriptor htd =
+      this.masterServices.getTableDescriptors().get(name);
+    if (htd == null) {
+      throw new IOException("HTableDescriptor missing for " + name);
+    }
+    return htd;
+  }
+
+  byte [] hasColumnFamily(final HTableDescriptor htd, final byte [] cf)
+  throws InvalidFamilyOperationException {
+    if (!htd.hasFamily(cf)) {
+      throw new InvalidFamilyOperationException("Column family '" +
+        Bytes.toString(cf) + "' does not exist");
+    }
+    return cf;
+  }
+
   protected abstract void handleTableOperation(List<HRegionInfo> regions)
   throws IOException, KeeperException;
 }

src/main/java/org/apache/hadoop/hbase/master/handler/TableModifyFamilyHandler.java

@@ -27,7 +27,6 @@ import org.apache.hadoop.hbase.HRegionInfo;
 import org.apache.hadoop.hbase.HTableDescriptor;
 import org.apache.hadoop.hbase.InvalidFamilyOperationException;
 import org.apache.hadoop.hbase.Server;
-import org.apache.hadoop.hbase.master.AssignmentManager;
 import org.apache.hadoop.hbase.master.MasterServices;
 import org.apache.hadoop.hbase.util.Bytes;
 
@@ -35,35 +34,26 @@ import org.apache.hadoop.hbase.util.Bytes;
  * Handles adding a new family to an existing table.
  */
 public class TableModifyFamilyHandler extends TableEventHandler {
-
   private final HColumnDescriptor familyDesc;
 
   public TableModifyFamilyHandler(byte[] tableName,
       HColumnDescriptor familyDesc, Server server,
       final MasterServices masterServices) throws IOException {
     super(EventType.C_M_MODIFY_FAMILY, tableName, server, masterServices);
+    HTableDescriptor htd = getTableDescriptor();
+    hasColumnFamily(htd, familyDesc.getName());
     this.familyDesc = familyDesc;
   }
 
   @Override
   protected void handleTableOperation(List<HRegionInfo> regions) throws IOException {
-    AssignmentManager am = this.masterServices.getAssignmentManager();
-    HTableDescriptor htd = this.masterServices.getTableDescriptors().get(Bytes.toString(tableName));
-    byte [] familyName = familyDesc.getName();
-    if (htd == null) {
-      throw new IOException("Modify Family operation could not be completed as " +
-          "HTableDescritor is missing for table = "
-          + Bytes.toString(tableName));
-    }
-    if(!htd.hasFamily(familyName)) {
-      throw new InvalidFamilyOperationException("Family '" +
-        Bytes.toString(familyName) + "' doesn't exists so cannot be modified");
-    }
     // Update table descriptor in HDFS
-    htd = this.masterServices.getMasterFileSystem().modifyColumn(tableName, familyDesc);
+    HTableDescriptor htd =
+      this.masterServices.getMasterFileSystem().modifyColumn(tableName, familyDesc);
     // Update in-memory descriptor cache
     this.masterServices.getTableDescriptors().add(htd);
   }
+
   @Override
   public String toString() {
     String name = "UnknownServerName";

src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java

@@ -47,6 +47,7 @@ import org.apache.hadoop.hbase.master.MasterServices;
 import org.apache.hadoop.hbase.regionserver.HRegion;
 import org.apache.hadoop.hbase.regionserver.HRegionServer;
 import org.apache.hadoop.hbase.regionserver.wal.TestHLogUtils;
+import org.apache.hadoop.hbase.InvalidFamilyOperationException;
 import org.apache.hadoop.hbase.util.Bytes;
 import org.junit.AfterClass;
 import org.junit.Before;
@@ -86,6 +87,96 @@ public class TestAdmin {
     this.admin = new HBaseAdmin(TEST_UTIL.getConfiguration());
   }
 
+  @Test
+  public void testDeleteEditUnknownColumnFamilyAndOrTable() throws IOException {
+    // Test we get exception if we try to 
+    final String nonexistent = "nonexistent";
+    HColumnDescriptor nonexistentHcd = new HColumnDescriptor(nonexistent);
+    Exception exception = null;
+    try {
+      this.admin.addColumn(nonexistent, nonexistentHcd);
+    } catch (IOException e) {
+      exception = e;
+    }
+    assertTrue(exception instanceof TableNotFoundException);
+
+    exception = null;
+    try {
+      this.admin.deleteTable(nonexistent);
+    } catch (IOException e) {
+      exception = e;
+    }
+    assertTrue(exception instanceof TableNotFoundException);
+
+    exception = null;
+    try {
+      this.admin.deleteColumn(nonexistent, nonexistent);
+    } catch (IOException e) {
+      exception = e;
+    }
+    assertTrue(exception instanceof TableNotFoundException);
+
+    exception = null;
+    try {
+      this.admin.disableTable(nonexistent);
+    } catch (IOException e) {
+      exception = e;
+    }
+    assertTrue(exception instanceof TableNotFoundException);
+
+    exception = null;
+    try {
+      this.admin.enableTable(nonexistent);
+    } catch (IOException e) {
+      exception = e;
+    }
+    assertTrue(exception instanceof TableNotFoundException);
+
+    exception = null;
+    try {
+      this.admin.modifyColumn(nonexistent, nonexistentHcd);
+    } catch (IOException e) {
+      exception = e;
+    }
+    assertTrue(exception instanceof TableNotFoundException);
+
+    exception = null;
+    try {
+      HTableDescriptor htd = new HTableDescriptor(nonexistent);
+      this.admin.modifyTable(htd.getName(), htd);
+    } catch (IOException e) {
+      exception = e;
+    }
+    assertTrue(exception instanceof TableNotFoundException);
+
+    // Now make it so at least the table exists and then do tests against a
+    // nonexistent column family -- see if we get right exceptions.
+    final String tableName = "t";
+    HTableDescriptor htd = new HTableDescriptor(tableName);
+    htd.addFamily(new HColumnDescriptor("cf"));
+    this.admin.createTable(htd);
+    try {
+      exception = null;
+      try {
+        this.admin.deleteColumn(htd.getName(), nonexistentHcd.getName());
+      } catch (IOException e) {
+        exception = e;
+      }
+      assertTrue(exception instanceof InvalidFamilyOperationException);
+
+      exception = null;
+      try {
+        this.admin.modifyColumn(htd.getName(), nonexistentHcd);
+      } catch (IOException e) {
+        exception = e;
+      }
+      assertTrue(exception instanceof InvalidFamilyOperationException);
+    } finally {
+      this.admin.disableTable(tableName);
+      this.admin.deleteTable(tableName);
+    }
+  }
+
   @Test
   public void testDisableAndEnableTable() throws IOException {
     final byte [] row = Bytes.toBytes("row");
@@ -218,7 +309,6 @@ public class TestAdmin {
   public void testOnlineChangeTableSchema() throws IOException, InterruptedException {
     final byte [] tableName = Bytes.toBytes("changeTableSchemaOnline");
     HTableDescriptor [] tables = admin.listTables();
-    MasterServices masterServices = TEST_UTIL.getMiniHBaseCluster().getMaster();
     int numTables = tables.length;
     TEST_UTIL.createTable(tableName, HConstants.CATALOG_FAMILY);
     tables = this.admin.listTables();
@@ -243,7 +333,6 @@ public class TestAdmin {
     }
     assertFalse(expectedException);
     HTableDescriptor modifiedHtd = this.admin.getTableDescriptor(tableName);
-    // Assert returned modifiedhcd is same as the copy.
     assertFalse(htd.equals(modifiedHtd));
     assertTrue(copy.equals(modifiedHtd));
     assertEquals(newFlushSize, modifiedHtd.getMemStoreFlushSize());
@@ -784,7 +873,7 @@ public class TestAdmin {
 
   @Test (expected=IllegalArgumentException.class)
   public void testInvalidHColumnDescriptor() throws IOException {
-     HColumnDescriptor hcd = new HColumnDescriptor("/cfamily/name");
+     new HColumnDescriptor("/cfamily/name");
   }
 
   @Test
@@ -801,11 +890,10 @@ public class TestAdmin {
     this.admin.enableTable(tableName);
     try {
       this.admin.deleteColumn(tableName, Bytes.toBytes("col2"));
-    } catch(TableNotDisabledException e) {
-      // Expected
+    } catch (TableNotDisabledException e) {
+      LOG.info(e);
     }
     this.admin.disableTable(tableName);
-    this.admin.deleteColumn(tableName, Bytes.toBytes("col2"));
     this.admin.deleteTable(tableName);
   }