HBASE-12329 Table create with duplicate column family names quietly succeeds (Jingcheng Du)

hbase-client/src/main/java/org/apache/hadoop/hbase/HTableDescriptor.java

@@ -767,11 +767,33 @@ public class HTableDescriptor implements Comparable<HTableDescriptor> {
 
   /**
    * Adds a column family.
+   * For the updating purpose please use {@link #modifyFamily(HColumnDescriptor)} instead.
    * @param family HColumnDescriptor of family to add.
    */
   public HTableDescriptor addFamily(final HColumnDescriptor family) {
     if (family.getName() == null || family.getName().length <= 0) {
-      throw new NullPointerException("Family name cannot be null or empty");
+      throw new IllegalArgumentException("Family name cannot be null or empty");
+    }
+    if (hasFamily(family.getName())) {
+      throw new IllegalArgumentException("Family '" +
+        family.getNameAsString() + "' already exists so cannot be added");
+    }
+    this.families.put(family.getName(), family);
+    return this;
+  }
+
+  /**
+   * Modifies the existing column family.
+   * @param family HColumnDescriptor of family to update
+   * @return this (for chained invocation)
+   */
+  public HTableDescriptor modifyFamily(final HColumnDescriptor family) {
+    if (family.getName() == null || family.getName().length <= 0) {
+      throw new IllegalArgumentException("Family name cannot be null or empty");
+    }
+    if (!hasFamily(family.getName())) {
+      throw new IllegalArgumentException("Column family '" + family.getNameAsString()
+        + "' does not exist");
     }
     this.families.put(family.getName(), family);
     return this;

hbase-client/src/main/java/org/apache/hadoop/hbase/client/UnmodifyableHTableDescriptor.java

@@ -67,6 +67,11 @@ public class UnmodifyableHTableDescriptor extends HTableDescriptor {
     throw new UnsupportedOperationException("HTableDescriptor is read-only");
   }
 
+  @Override
+  public HTableDescriptor modifyFamily(HColumnDescriptor family) {
+    throw new UnsupportedOperationException("HTableDescriptor is read-only");
+  }
+
   /**
    * @param column
    * @return Column descriptor for the passed family name or the family on

hbase-client/src/test/java/org/apache/hadoop/hbase/TestHTableDescriptor.java

@@ -225,4 +225,39 @@ public class TestHTableDescriptor {
 
     BuilderStyleTest.assertClassesAreBuilderStyle(HTableDescriptor.class);
   }
+
+  @Test
+  public void testModifyFamily() {
+    HTableDescriptor htd = new HTableDescriptor(TableName.valueOf("table"));
+    byte[] familyName = Bytes.toBytes("cf");
+    HColumnDescriptor hcd = new HColumnDescriptor(familyName);
+    hcd.setBlocksize(1000);
+    htd.addFamily(hcd);
+    assertEquals(1000, htd.getFamily(familyName).getBlocksize());
+    hcd = new HColumnDescriptor(familyName);
+    hcd.setBlocksize(2000);
+    htd.modifyFamily(hcd);
+    assertEquals(2000, htd.getFamily(familyName).getBlocksize());
+  }
+
+  @Test(expected=IllegalArgumentException.class)
+  public void testModifyInexistentFamily() {
+    HTableDescriptor htd = new HTableDescriptor(TableName.valueOf("table"));
+    byte[] familyName = Bytes.toBytes("cf");
+    HColumnDescriptor hcd = new HColumnDescriptor(familyName);
+    htd.modifyFamily(hcd);
+  }
+
+  @Test(expected=IllegalArgumentException.class)
+  public void testAddDuplicateFamilies() {
+    HTableDescriptor htd = new HTableDescriptor(TableName.valueOf("table"));
+    byte[] familyName = Bytes.toBytes("cf");
+    HColumnDescriptor hcd = new HColumnDescriptor(familyName);
+    hcd.setBlocksize(1000);
+    htd.addFamily(hcd);
+    assertEquals(1000, htd.getFamily(familyName).getBlocksize());
+    hcd = new HColumnDescriptor(familyName);
+    hcd.setBlocksize(2000);
+    htd.addFamily(hcd);
+  }
 }

hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java

@@ -621,7 +621,7 @@ public class MasterFileSystem {
       throw new InvalidFamilyOperationException("Family '" +
         Bytes.toString(familyName) + "' doesn't exists so cannot be modified");
     }
-    htd.addFamily(hcd);
+    htd.modifyFamily(hcd);
     this.services.getTableDescriptors().add(htd);
     return htd;
   }

hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java

@@ -222,7 +222,7 @@ public class TestFromClientSide3 {
     LOG.info("hbase.hstore.compaction.min should now be 2");
     HColumnDescriptor hcd = new HColumnDescriptor(htd.getFamily(FAMILY));
     hcd.setValue("hbase.hstore.compaction.min", String.valueOf(2));
-    htd.addFamily(hcd);
+    htd.modifyFamily(hcd);
     admin.modifyTable(TABLE, htd);
     while (null != (st = admin.getAlterStatus(TABLE)) && st.getFirst() > 0) {
       LOG.debug(st.getFirst() + " regions left to update");
@@ -257,7 +257,7 @@ public class TestFromClientSide3 {
     LOG.info("hbase.hstore.compaction.min should now be 5");
     hcd = new HColumnDescriptor(htd.getFamily(FAMILY));
     hcd.setValue("hbase.hstore.compaction.min", null);
-    htd.addFamily(hcd);
+    htd.modifyFamily(hcd);
     admin.modifyTable(TABLE, htd);
     while (null != (st = admin.getAlterStatus(TABLE)) && st.getFirst() > 0) {
       LOG.debug(st.getFirst() + " regions left to update");

hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFilter.java

@@ -141,7 +141,6 @@ public class TestFilter {
     htd.addFamily(new HColumnDescriptor(FAMILIES_1[1]));
     htd.addFamily(new HColumnDescriptor(NEW_FAMILIES[0]));
     htd.addFamily(new HColumnDescriptor(NEW_FAMILIES[1]));
-    htd.addFamily(new HColumnDescriptor(FAMILIES_1[1]));
     HRegionInfo info = new HRegionInfo(htd.getTableName(), null, null, false);
     this.region = HRegion.createHRegion(info, TEST_UTIL.getDataTestDir(),
             TEST_UTIL.getConfiguration(), htd);

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRowTooBig.java

@@ -73,7 +73,11 @@ public class TestRowTooBig {
 
     HTableDescriptor htd = TEST_HTD;
     HColumnDescriptor hcd = new HColumnDescriptor(fam1);
+    if (htd.hasFamily(hcd.getName())) {
+      htd.modifyFamily(hcd);
+    } else {
       htd.addFamily(hcd);
+    }
 
     final HRegionInfo hri =
       new HRegionInfo(htd.getTableName(), HConstants.EMPTY_END_ROW,
@@ -111,7 +115,11 @@ public class TestRowTooBig {
 
     HTableDescriptor htd = TEST_HTD;
     HColumnDescriptor hcd = new HColumnDescriptor(fam1);
+    if (htd.hasFamily(hcd.getName())) {
+      htd.modifyFamily(hcd);
+    } else {
       htd.addFamily(hcd);
+    }
 
     final HRegionInfo hri =
       new HRegionInfo(htd.getTableName(), HConstants.EMPTY_END_ROW,

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java

@@ -175,7 +175,11 @@ public class TestStore {
 
     fs.delete(logdir, true);
 
+    if (htd.hasFamily(hcd.getName())) {
+      htd.modifyFamily(hcd);
+    } else {
       htd.addFamily(hcd);
+    }
     HRegionInfo info = new HRegionInfo(htd.getTableName(), null, null, false);
     HLog hlog = HLogFactory.createHLog(fs, basedir, logName, conf);
     HRegion region = new HRegion(tableDir, hlog, fs, conf, info, htd, null);

hbase-server/src/test/java/org/apache/hadoop/hbase/snapshot/SnapshotTestingUtils.java

@@ -554,7 +554,6 @@ public class SnapshotTestingUtils {
     private SnapshotBuilder createSnapshot(final String snapshotName, final int version)
         throws IOException {
       HTableDescriptor htd = createHtd(snapshotName);
-      htd.addFamily(new HColumnDescriptor(TEST_FAMILY));
 
       RegionData[] regions = createTable(htd, TEST_NUM_REGIONS);
 

hbase-shell/src/main/ruby/hbase/admin.rb

@@ -217,7 +217,14 @@ module Hbase
         if arg.kind_of?(String) or arg.has_key?(NAME)
           # If the arg is a string, default action is to add a column to the table.
           # If arg has a name, it must also be a column descriptor.
-          htd.addFamily(hcd(arg, htd))
+          descriptor = hcd(arg, htd);
+          # Warn if duplicate columns are added
+          if htd.hasFamily(descriptor.getName)
+            puts "Family '" + descriptor.getNameAsString() + "' already exists, the old one will be replaced"
+            htd.modifyFamily(descriptor)
+          else
+            htd.addFamily(descriptor)
+          end
           has_columns = true
           next
         end