HBASE-18306 Get rid of TableDescriptor#getConfiguration

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

@@ -815,17 +815,19 @@ public class HTableDescriptor implements TableDescriptor, Comparable<HTableDescr
   /**
    * Getter for accessing the configuration value by key
    */
-  @Override
   public String getConfigurationValue(String key) {
-    return delegatee.getConfigurationValue(key);
+    return delegatee.getValue(key);
   }
 
   /**
    * Getter for fetching an unmodifiable map.
    */
-  @Override
   public Map<String, String> getConfiguration() {
-    return delegatee.getConfiguration();
+    return delegatee.getValues().entrySet().stream()
+            .collect(Collectors.toMap(
+                    e -> Bytes.toString(e.getKey().get(), e.getKey().getOffset(), e.getKey().getLength()),
+                    e -> Bytes.toString(e.getValue().get(), e.getValue().getOffset(), e.getValue().getLength())
+            ));
   }
 
   /**
@@ -834,7 +836,7 @@ public class HTableDescriptor implements TableDescriptor, Comparable<HTableDescr
    * @param value String value. If null, removes the setting.
    */
   public HTableDescriptor setConfiguration(String key, String value) {
-    getDelegateeForModification().setConfiguration(key, value);
+    getDelegateeForModification().setValue(key, value);
     return this;
   }
 
@@ -842,7 +844,7 @@ public class HTableDescriptor implements TableDescriptor, Comparable<HTableDescr
    * Remove a config setting represented by the key from the map
    */
   public void removeConfiguration(final String key) {
-    getDelegateeForModification().removeConfiguration(key);
+    getDelegateeForModification().removeValue(Bytes.toBytes(key));
   }
 
   @Override

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

@@ -60,11 +60,7 @@ public interface TableDescriptor {
         }
       }
       // punt on comparison for ordering, just calculate difference
-      result = Integer.compare(lhs.getValues().hashCode(), rhs.getValues().hashCode());
+      return Integer.compare(lhs.getValues().hashCode(), rhs.getValues().hashCode());
-      if (result != 0) {
-        return result;
-      }
-      return Integer.compare(lhs.getConfiguration().hashCode(), rhs.getConfiguration().hashCode());
   };
 
   /**
@@ -74,22 +70,6 @@ public interface TableDescriptor {
    */
   int getColumnFamilyCount();
 
-  /**
-   * Getter for fetching an unmodifiable map.
-   *
-   * @return an unmodifiable map
-   */
-  Map<String, String> getConfiguration();
-
-  /**
-   * Getter for accessing the configuration value by key
-   *
-   * @param key the key whose associated value is to be returned
-   * @return the value to which the specified key is mapped, or {@code null} if
-   * this map contains no mapping for the key
-   */
-  String getConfigurationValue(String key);
-
   /**
    * Return the list of attached co-processor represented by their name
    * className

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

@@ -38,7 +38,6 @@ import org.apache.commons.logging.LogFactory;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.hbase.Coprocessor;
 import org.apache.hadoop.hbase.HConstants;
-import org.apache.hadoop.hbase.HTableDescriptor;
 import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.classification.InterfaceAudience;
 import org.apache.hadoop.hbase.exceptions.DeserializationException;
@@ -137,6 +136,8 @@ public class TableDescriptorBuilder {
   private static final Bytes REGION_MEMSTORE_REPLICATION_KEY
           = new Bytes(Bytes.toBytes(REGION_MEMSTORE_REPLICATION));
 
+  private static final Bytes REGION_REPLICA_WAIT_FOR_PRIMARY_FLUSH_CONF_KEY
+          = new Bytes(Bytes.toBytes(RegionReplicaUtil.REGION_REPLICA_WAIT_FOR_PRIMARY_FLUSH_CONF_KEY));
   /**
    * Used by shell/rest interface to access this metadata
    * attribute which denotes if the table should be treated by region
@@ -265,7 +266,7 @@ public class TableDescriptorBuilder {
   }
 
   /**
-   * Copy all configuration, values, families, and name from the input.
+   * Copy all values, families, and name from the input.
    * @param desc The desciptor to copy
    * @return A clone of input
    */
@@ -316,11 +317,6 @@ public class TableDescriptorBuilder {
     return this;
   }
 
-  public TableDescriptorBuilder removeConfiguration(final String key) {
-    desc.removeConfiguration(key);
-    return this;
-  }
-
   public TableDescriptorBuilder removeColumnFamily(final byte[] name) {
     desc.removeColumnFamily(name);
     return this;
@@ -336,11 +332,6 @@ public class TableDescriptorBuilder {
     return this;
   }
 
-  public TableDescriptorBuilder setConfiguration(String key, String value) {
-    desc.setConfiguration(key, value);
-    return this;
-  }
-
   public TableDescriptorBuilder setDurability(Durability durability) {
     desc.setDurability(durability);
     return this;
@@ -403,6 +394,11 @@ public class TableDescriptorBuilder {
     return this;
   }
 
+  public TableDescriptorBuilder setValue(final String key, final String value) {
+    desc.setValue(key, value);
+    return this;
+  }
+
   public TableDescriptorBuilder setValue(final Bytes key, final Bytes value) {
     desc.setValue(key, value);
     return this;
@@ -433,13 +429,6 @@ public class TableDescriptorBuilder {
      */
     private final Map<Bytes, Bytes> values = new HashMap<>();
 
-    /**
-     * A map which holds the configuration specific to the table. The keys of
-     * the map have the same names as config keys and override the defaults with
-     * table-specific settings. Example usage may be for compactions, etc.
-     */
-    private final Map<String, String> configuration = new HashMap<>();
-
     /**
      * Maps column family name to the respective FamilyDescriptors
      */
@@ -454,11 +443,11 @@ public class TableDescriptorBuilder {
      */
     @InterfaceAudience.Private
     public ModifyableTableDescriptor(final TableName name) {
-      this(name, Collections.EMPTY_LIST, Collections.EMPTY_MAP, Collections.EMPTY_MAP);
+      this(name, Collections.EMPTY_LIST, Collections.EMPTY_MAP);
     }
 
     private ModifyableTableDescriptor(final TableDescriptor desc) {
-      this(desc.getTableName(), Arrays.asList(desc.getColumnFamilies()), desc.getValues(), desc.getConfiguration());
+      this(desc.getTableName(), Arrays.asList(desc.getColumnFamilies()), desc.getValues());
     }
 
     /**
@@ -473,15 +462,14 @@ public class TableDescriptorBuilder {
     @InterfaceAudience.Private
     @Deprecated // only used by HTableDescriptor. remove this method if HTD is removed
     public ModifyableTableDescriptor(final TableName name, final TableDescriptor desc) {
-      this(name, Arrays.asList(desc.getColumnFamilies()), desc.getValues(), desc.getConfiguration());
+      this(name, Arrays.asList(desc.getColumnFamilies()), desc.getValues());
     }
 
     private ModifyableTableDescriptor(final TableName name, final Collection<ColumnFamilyDescriptor> families,
-            Map<Bytes, Bytes> values, Map<String, String> configuration) {
+            Map<Bytes, Bytes> values) {
       this.name = name;
       families.forEach(c -> this.families.put(c.getName(), ColumnFamilyDescriptorBuilder.copy(c)));
       this.values.putAll(values);
-      this.configuration.putAll(configuration);
       this.values.put(IS_META_KEY,
         new Bytes(Bytes.toBytes(Boolean.toString(name.equals(TableName.META_TABLE_NAME)))));
     }
@@ -558,6 +546,11 @@ public class TableDescriptorBuilder {
               toBytesOrNull(value, v -> v));
     }
 
+    public ModifyableTableDescriptor setValue(String key, String value) {
+      return setValue(toBytesOrNull(key, Bytes::toBytes),
+              toBytesOrNull(value, Bytes::toBytes));
+    }
+
     /*
      * @param key The key.
      * @param value The value. If null, removes the setting.
@@ -936,7 +929,7 @@ public class TableDescriptorBuilder {
 
       // early exit optimization
       boolean hasAttributes = !reservedKeys.isEmpty() || !userKeys.isEmpty();
-      if (!hasAttributes && configuration.isEmpty()) {
+      if (!hasAttributes) {
         return s;
       }
 
@@ -960,7 +953,7 @@ public class TableDescriptorBuilder {
         }
 
         if (!userKeys.isEmpty()) {
-          // print all non-reserved, advanced config keys as a separate subset
+          // print all non-reserved as a separate subset
           if (printCommaForAttr) {
             s.append(", ");
           }
@@ -982,25 +975,6 @@ public class TableDescriptorBuilder {
         }
       }
 
-      // step 3: printing all configuration:
-      if (!configuration.isEmpty()) {
-        if (hasAttributes) {
-          s.append(", ");
-        }
-        s.append(HConstants.CONFIGURATION).append(" => ");
-        s.append('{');
-        boolean printCommaForConfig = false;
-        for (Map.Entry<String, String> e : configuration.entrySet()) {
-          if (printCommaForConfig) {
-            s.append(", ");
-          }
-          printCommaForConfig = true;
-          s.append('\'').append(e.getKey()).append('\'');
-          s.append(" => ");
-          s.append('\'').append(e.getValue()).append('\'');
-        }
-        s.append("}");
-      }
       s.append("}"); // end METHOD
       return s;
     }
@@ -1038,7 +1012,6 @@ public class TableDescriptorBuilder {
         }
       }
       result ^= values.hashCode();
-      result ^= configuration.hashCode();
       return result;
     }
 
@@ -1112,7 +1085,7 @@ public class TableDescriptorBuilder {
       setValue(REGION_MEMSTORE_REPLICATION_KEY, Boolean.toString(memstoreReplication));
       // If the memstore replication is setup, we do not have to wait for observing a flush event
       // from primary before starting to serve reads, because gaps from replication is not applicable
-      return setConfiguration(RegionReplicaUtil.REGION_REPLICA_WAIT_FOR_PRIMARY_FLUSH_CONF_KEY,
+      return setValue(REGION_REPLICA_WAIT_FOR_PRIMARY_FLUSH_CONF_KEY,
               Boolean.toString(memstoreReplication));
     }
 
@@ -1426,50 +1399,6 @@ public class TableDescriptorBuilder {
       }
     }
 
-    /**
-     * Getter for accessing the configuration value by key
-     */
-    @Override
-    public String getConfigurationValue(String key) {
-      return configuration.get(key);
-    }
-
-    /**
-     * Getter for fetching an unmodifiable {@link #configuration} map.
-     */
-    @Override
-    public Map<String, String> getConfiguration() {
-      // shallow pointer copy
-      return Collections.unmodifiableMap(configuration);
-    }
-
-    /**
-     * Setter for storing a configuration setting in {@link #configuration} map.
-     *
-     * @param key Config key. Same as XML config key e.g.
-     * hbase.something.or.other.
-     * @param value String value. If null, removes the setting.
-     * @return the modifyable TD
-     */
-    public ModifyableTableDescriptor setConfiguration(String key, String value) {
-      if (value == null) {
-        configuration.remove(key);
-      } else {
-        configuration.put(key, value);
-      }
-      return this;
-    }
-
-    /**
-     * Remove a config setting represented by the key from the
-     * {@link #configuration} map
-     * @param key Config key.
-     * @return the modifyable TD
-     */
-    public ModifyableTableDescriptor removeConfiguration(final String key) {
-      return setConfiguration(key, null);
-    }
-
     @Override
     public int getColumnFamilyCount() {
       return families.size();

hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java

@@ -2817,12 +2817,6 @@ public final class ProtobufUtil {
     for (ColumnFamilyDescriptor hcd : htd.getColumnFamilies()) {
       builder.addColumnFamilies(toColumnFamilySchema(hcd));
     }
-    for (Map.Entry<String, String> e : htd.getConfiguration().entrySet()) {
-      NameStringPair.Builder aBuilder = NameStringPair.newBuilder();
-      aBuilder.setName(e.getKey());
-      aBuilder.setValue(e.getValue());
-      builder.addConfiguration(aBuilder.build());
-    }
     return builder.build();
   }
 
@@ -2841,7 +2835,7 @@ public final class ProtobufUtil {
     ts.getAttributesList()
       .forEach(a -> builder.setValue(a.getFirst().toByteArray(), a.getSecond().toByteArray()));
     ts.getConfigurationList()
-      .forEach(a -> builder.setConfiguration(a.getName(), a.getValue()));
+      .forEach(a -> builder.setValue(a.getName(), a.getValue()));
     return builder.build();
   }
 

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

@@ -271,25 +271,6 @@ public class TestTableDescriptorBuilder {
     assertEquals(1111L, desc.getMemStoreFlushSize());
   }
 
-  /**
-   * Test that we add and remove strings from configuration properly.
-   */
-  @Test
-  public void testAddGetRemoveConfiguration() {
-    String key = "Some";
-    String value = "value";
-    TableDescriptor desc = TableDescriptorBuilder
-            .newBuilder(TableName.valueOf(name.getMethodName()))
-            .setConfiguration(key, value)
-            .build();
-    assertEquals(value, desc.getConfigurationValue(key));
-    desc = TableDescriptorBuilder
-            .newBuilder(desc)
-            .removeConfiguration(key)
-            .build();
-    assertEquals(null, desc.getConfigurationValue(key));
-  }
-
   @Test
   public void testClassMethodsAreBuilderStyle() {
     BuilderStyleTest.assertClassesAreBuilderStyle(TableDescriptorBuilder.class);

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

@@ -1800,7 +1800,7 @@ public class HMaster extends HRegionServer implements MasterServices {
     if (!conf.getBoolean(CONF_KEY, true)) {
       logWarn = true;
     }
-    String tableVal = htd.getConfigurationValue(CONF_KEY);
+    String tableVal = htd.getValue(CONF_KEY);
     if (tableVal != null && !Boolean.valueOf(tableVal)) {
       logWarn = true;
     }
@@ -1925,8 +1925,7 @@ public class HMaster extends HRegionServer implements MasterServices {
       throws IOException {
     // FIFO compaction has some requirements
     // Actually FCP ignores periodic major compactions
-    String className =
+    String className = htd.getValue(DefaultStoreEngine.DEFAULT_COMPACTION_POLICY_CLASS_KEY);
-        htd.getConfigurationValue(DefaultStoreEngine.DEFAULT_COMPACTION_POLICY_CLASS_KEY);
     if (className == null) {
       className =
           conf.get(DefaultStoreEngine.DEFAULT_COMPACTION_POLICY_CLASS_KEY,
@@ -1934,7 +1933,7 @@ public class HMaster extends HRegionServer implements MasterServices {
     }
 
     int blockingFileCount = HStore.DEFAULT_BLOCKING_STOREFILE_COUNT;
-    String sv = htd.getConfigurationValue(HStore.BLOCKING_STOREFILES_KEY);
+    String sv = htd.getValue(HStore.BLOCKING_STOREFILES_KEY);
     if (sv != null) {
       blockingFileCount = Integer.parseInt(sv);
     } else {

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java

@@ -721,7 +721,6 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi
     this.baseConf = confParam;
     this.conf = new CompoundConfiguration()
       .add(confParam)
-      .addStringMap(htd.getConfiguration())
       .addBytesMap(htd.getValues());
     this.flushCheckInterval = conf.getInt(MEMSTORE_PERIODIC_FLUSH_INTERVAL,
         DEFAULT_CACHE_FLUSH_INTERVAL);

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java

@@ -229,7 +229,7 @@ public class HStore implements Store {
     // add global config first, then table and cf overrides, then cf metadata.
     this.conf = new CompoundConfiguration()
       .add(confParam)
-      .addStringMap(region.getTableDescriptor().getConfiguration())
+      .addBytesMap(region.getTableDescriptor().getValues())
       .addStringMap(family.getConfiguration())
       .addBytesMap(family.getValues());
     this.blocksize = family.getBlocksize();

hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java

@@ -954,7 +954,7 @@ public class AccessController implements MasterObserver, RegionObserver, RegionS
     } else if (env instanceof RegionCoprocessorEnvironment) {
       // if running at region
       regionEnv = (RegionCoprocessorEnvironment) env;
-      conf.addStringMap(regionEnv.getRegion().getTableDescriptor().getConfiguration());
+      conf.addBytesMap(regionEnv.getRegion().getTableDescriptor().getValues());
       zk = regionEnv.getRegionServerServices().getZooKeeper();
       compatibleEarlyTermination = conf.getBoolean(AccessControlConstants.CF_ATTRIBUTE_EARLY_OUT,
         AccessControlConstants.DEFAULT_ATTRIBUTE_EARLY_OUT);

hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestCreateTableProcedure.java

@@ -81,7 +81,7 @@ public class TestCreateTableProcedure extends TestTableDDLProcedureBase {
     final TableDescriptorBuilder builder = TableDescriptorBuilder.newBuilder(MasterProcedureTestingUtility.createHTD(tableName));
 
     // disable sanity check
-    builder.setConfiguration("hbase.table.sanity.checks", Boolean.FALSE.toString());
+    builder.setValue("hbase.table.sanity.checks", Boolean.FALSE.toString());
     TableDescriptor htd = builder.build();
     final HRegionInfo[] regions = ModifyRegionUtils.createHRegionInfos(htd, null);
 

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

@@ -192,7 +192,7 @@ public class TestRegionSnapshotTask {
         .setMemStoreFlushSize(5000)
         .setRegionSplitPolicyClassName(ConstantSizeRegionSplitPolicy.class.getName())
         .setMaxFileSize(100 * 1024 * 1024)
-        .setConfiguration("hbase.hstore.compactionThreshold", "250");
+        .setValue("hbase.hstore.compactionThreshold", "250");
 
     TableDescriptor td = builder.build();
     byte[] fam = Bytes.toBytes("fam");