HBASE-30192 ColumnFamilyDescriptor.getConfigurationValue should also read the values map

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

@@ -99,7 +99,10 @@ public interface ColumnFamilyDescriptor {
   /** Returns an unmodifiable map. */
   Map<String, String> getConfiguration();
 
-  /** Returns accessing the configuration value by key. */
+  /**
+   * Returns the value for the given key, looking in the values map (where the shell writes settings
+   * since HBASE-20819) and the legacy configuration map.
+   */
   String getConfigurationValue(String key);
 
   /** Returns replication factor set for this CF */

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

@@ -1289,7 +1289,11 @@ private static ColumnFamilyDescriptor parseFrom(final byte[] bytes)
 
     @Override
     public String getConfigurationValue(String key) {
-      return configuration.get(key);
+      // Fall back to the values map (where the shell writes settings since HBASE-20819) so a
+      // single-key read sees a setting regardless of which map it landed in. Values win on
+      // collision.
+      String value = getValue(key);
+      return value != null ? value : configuration.get(key);
     }
 
     @Override

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

@@ -114,6 +114,26 @@ public void testAddGetRemoveConfiguration() {
     assertEquals(null, builder.build().getConfigurationValue(key));
   }
 
+  @Test
+  public void testGetConfigurationValueFallsBackToValues() {
+    // The shell writes settings into the values map (HBASE-20819). getConfigurationValue must see
+    // them, not just keys set via setConfiguration.
+    String key = "some.key";
+    ColumnFamilyDescriptor viaValue =
+      ColumnFamilyDescriptorBuilder.newBuilder(Bytes.toBytes("foo")).setValue(key, "v").build();
+    assertEquals("v", viaValue.getConfigurationValue(key));
+
+    ColumnFamilyDescriptor viaConf = ColumnFamilyDescriptorBuilder.newBuilder(Bytes.toBytes("foo"))
+      .setConfiguration(key, "c").build();
+    assertEquals("c", viaConf.getConfigurationValue(key));
+
+    // Values win on collision.
+    ColumnFamilyDescriptor both = ColumnFamilyDescriptorBuilder.newBuilder(Bytes.toBytes("foo"))
+      .setConfiguration(key, "c").setValue(key, "v").build();
+    assertEquals("v", both.getConfigurationValue(key));
+    assertNull(both.getConfigurationValue("absent"));
+  }
+
   @Test
   public void testMobValuesInHColumnDescriptorShouldReadable() {
     boolean isMob = true;

hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestTableDescriptorChecker.java

@@ -27,6 +27,9 @@
 import org.apache.hadoop.hbase.client.TableDescriptorBuilder;
 import org.apache.hadoop.hbase.conf.ConfigKey;
 import org.apache.hadoop.hbase.regionserver.BloomType;
+import org.apache.hadoop.hbase.regionserver.DefaultStoreEngine;
+import org.apache.hadoop.hbase.regionserver.HStore;
+import org.apache.hadoop.hbase.regionserver.compactions.FIFOCompactionPolicy;
 import org.apache.hadoop.hbase.testclassification.MiscTests;
 import org.apache.hadoop.hbase.testclassification.SmallTests;
 import org.junit.jupiter.api.Tag;
@@ -119,4 +122,36 @@ public void testBloomFilterPrefixLengthValidation() throws IOException {
       TableDescriptorChecker.sanityCheck(conf, t.build());
     }
   }
+
+  @Test
+  public void testFifoCompactionPolicyValidation() throws IOException {
+    Configuration conf = new Configuration();
+    String key = DefaultStoreEngine.DEFAULT_COMPACTION_POLICY_CLASS_KEY;
+
+    // FIFO compaction requires a non-default TTL. The policy must be honored whether it is set on
+    // the column family via setValue (the shell path since HBASE-20819) or setConfiguration.
+    for (boolean viaSetValue : new boolean[] { true, false }) {
+      ColumnFamilyDescriptorBuilder cf = ColumnFamilyDescriptorBuilder.newBuilder("cf".getBytes());
+      TableDescriptorBuilder t = TableDescriptorBuilder.newBuilder(TableName.valueOf("test"));
+
+      if (viaSetValue) {
+        cf.setValue(key, FIFOCompactionPolicy.class.getName());
+        cf.setValue(HStore.BLOCKING_STOREFILES_KEY, "1000");
+      } else {
+        cf.setConfiguration(key, FIFOCompactionPolicy.class.getName());
+        cf.setConfiguration(HStore.BLOCKING_STOREFILES_KEY, "1000");
+      }
+      t.setColumnFamily(cf.build());
+      assertThrows(DoNotRetryIOException.class,
+        () -> TableDescriptorChecker.sanityCheck(conf, t.build()),
+        "Should reject FIFO compaction with default TTL set via "
+          + (viaSetValue ? "setValue" : "setConfiguration"));
+
+      // Fix the error: FIFO needs a finite TTL and no min versions.
+      cf.setTimeToLive(3600).setMinVersions(0);
+      t.removeColumnFamily("cf".getBytes());
+      t.setColumnFamily(cf.build());
+      TableDescriptorChecker.sanityCheck(conf, t.build());
+    }
+  }
 }