From fb2ba25dc1336713d03359ac8036266389404d2c Mon Sep 17 00:00:00 2001 From: Junegunn Choi Date: Mon, 1 Jun 2026 11:54:20 +0900 Subject: [PATCH] HBASE-30192 ColumnFamilyDescriptor.getConfigurationValue should also read the values map Since shell writes CF CONFIGURATION to values map (HBASE-20819), getConfigurationValue should access it before checking the legacy configuration map. Note: changes behavior of a public method. --- .../hbase/client/ColumnFamilyDescriptor.java | 5 ++- .../client/ColumnFamilyDescriptorBuilder.java | 6 +++- .../TestColumnFamilyDescriptorBuilder.java | 20 +++++++++++ .../util/TestTableDescriptorChecker.java | 35 +++++++++++++++++++ 4 files changed, 64 insertions(+), 2 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ColumnFamilyDescriptor.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ColumnFamilyDescriptor.java index ea8d81043694..e053dd9cc297 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ColumnFamilyDescriptor.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ColumnFamilyDescriptor.java @@ -99,7 +99,10 @@ public interface ColumnFamilyDescriptor { /** Returns an unmodifiable map. */ Map 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 */ diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ColumnFamilyDescriptorBuilder.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ColumnFamilyDescriptorBuilder.java index 6635645c0760..c0d9cfbfd9e8 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ColumnFamilyDescriptorBuilder.java +++ b/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 diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestColumnFamilyDescriptorBuilder.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestColumnFamilyDescriptorBuilder.java index f66328656ddf..a10e6994b260 100644 --- a/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestColumnFamilyDescriptorBuilder.java +++ b/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; diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestTableDescriptorChecker.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestTableDescriptorChecker.java index 17a36b43750d..9e32000d44b3 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestTableDescriptorChecker.java +++ b/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()); + } + } }