From 4d2705326426132e6ee170a4e71757c87e08cb78 Mon Sep 17 00:00:00 2001 From: Wellington Ramos Chevreuil Date: Wed, 13 Oct 2021 15:48:13 +0100 Subject: [PATCH] =?UTF-8?q?HBASE-26326=20CreateTableProcedure=20fails=20wh?= =?UTF-8?q?en=20FileBasedStoreFileTracker=E2=80=A6=20(#3721)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Amend HBASE-26326 - HBASE-21515 has been challenging to backport into this branch. For HBASE-26326, we actually only need the changes HBASE-21515 added to ReflectionUtils, so I had given up on attempting to bring in HBASE-21515 and am adding these required changes as an amend to HBASE-26326. Change-Id: I82e9e2f44a2b64fa0423531cd1775ce55a1e1607 Signed-off-by: Duo Zhang Signed-off-by: Josh Elser --- .../hadoop/hbase/util/ReflectionUtils.java | 23 +++++++++------ .../procedure/CreateTableProcedure.java | 6 ++-- .../hadoop/hbase/regionserver/StoreUtils.java | 11 ++++++++ .../FileBasedStoreFileTracker.java | 9 +++++- .../MigrationStoreFileTracker.java | 12 -------- .../storefiletracker/StoreFileTracker.java | 5 ++-- .../StoreFileTrackerBase.java | 9 ++---- .../StoreFileTrackerFactory.java | 28 +++++++++++-------- .../procedure/TestCreateTableProcedure.java | 16 +++++++++++ .../TestChangeStoreFileTracker.java | 2 +- .../TestStoreFileTracker.java | 2 +- 11 files changed, 76 insertions(+), 47 deletions(-) diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/ReflectionUtils.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/ReflectionUtils.java index a136846a922e..2b3b9afaa5c7 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/ReflectionUtils.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/ReflectionUtils.java @@ -83,15 +83,20 @@ public static Constructor findConstructor(Class type, Object... paramT boolean match = true; for (int i = 0; i < ctorParamTypes.length && match; ++i) { - Class paramType = paramTypes[i].getClass(); - match = (!ctorParamTypes[i].isPrimitive()) ? ctorParamTypes[i].isAssignableFrom(paramType) : - ((int.class.equals(ctorParamTypes[i]) && Integer.class.equals(paramType)) || - (long.class.equals(ctorParamTypes[i]) && Long.class.equals(paramType)) || - (double.class.equals(ctorParamTypes[i]) && Double.class.equals(paramType)) || - (char.class.equals(ctorParamTypes[i]) && Character.class.equals(paramType)) || - (short.class.equals(ctorParamTypes[i]) && Short.class.equals(paramType)) || - (boolean.class.equals(ctorParamTypes[i]) && Boolean.class.equals(paramType)) || - (byte.class.equals(ctorParamTypes[i]) && Byte.class.equals(paramType))); + if (paramTypes[i] == null) { + match = !ctorParamTypes[i].isPrimitive(); + } else { + Class paramType = paramTypes[i].getClass(); + match = (!ctorParamTypes[i].isPrimitive()) ? + ctorParamTypes[i].isAssignableFrom(paramType) : + ((int.class.equals(ctorParamTypes[i]) && Integer.class.equals(paramType)) || + (long.class.equals(ctorParamTypes[i]) && Long.class.equals(paramType)) || + (double.class.equals(ctorParamTypes[i]) && Double.class.equals(paramType)) || + (char.class.equals(ctorParamTypes[i]) && Character.class.equals(paramType)) || + (short.class.equals(ctorParamTypes[i]) && Short.class.equals(paramType)) || + (boolean.class.equals(ctorParamTypes[i]) && Boolean.class.equals(paramType)) || + (byte.class.equals(ctorParamTypes[i]) && Byte.class.equals(paramType))); + } } if (match) { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CreateTableProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CreateTableProcedure.java index d1047eea4bb2..71279aed6a59 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CreateTableProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CreateTableProcedure.java @@ -31,7 +31,6 @@ import org.apache.hadoop.hbase.client.RegionInfo; import org.apache.hadoop.hbase.client.RegionReplicaUtil; import org.apache.hadoop.hbase.client.TableDescriptor; -import org.apache.hadoop.hbase.client.TableDescriptorBuilder; import org.apache.hadoop.hbase.client.TableState; import org.apache.hadoop.hbase.master.MasterCoprocessorHost; import org.apache.hadoop.hbase.master.MasterFileSystem; @@ -272,9 +271,8 @@ private void preCreate(final MasterProcedureEnv env) (newRegions != null ? newRegions.size() : 0)); } - TableDescriptorBuilder builder = TableDescriptorBuilder.newBuilder(tableDescriptor); - StoreFileTrackerFactory.persistTrackerConfig(env.getMasterConfiguration(), builder); - tableDescriptor = builder.build(); + tableDescriptor = StoreFileTrackerFactory.updateWithTrackerConfigs(env.getMasterConfiguration(), + tableDescriptor); final MasterCoprocessorHost cpHost = env.getMasterCoprocessorHost(); if (cpHost != null) { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreUtils.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreUtils.java index 30b70c1119ed..a8f754eb9d66 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreUtils.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreUtils.java @@ -31,7 +31,10 @@ import org.apache.hadoop.hbase.Cell; import org.apache.hadoop.hbase.CellComparator; import org.apache.hadoop.hbase.CellUtil; +import org.apache.hadoop.hbase.CompoundConfiguration; import org.apache.hadoop.hbase.HConstants; +import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor; +import org.apache.hadoop.hbase.client.TableDescriptor; import org.apache.hadoop.hbase.io.hfile.HFile; import org.apache.hadoop.hbase.util.ChecksumType; import org.apache.hadoop.hbase.CompoundConfiguration; @@ -170,6 +173,14 @@ public static int getBytesPerChecksum(Configuration conf) { HFile.DEFAULT_BYTES_PER_CHECKSUM); } + public static Configuration createStoreConfiguration(Configuration conf, TableDescriptor td, + ColumnFamilyDescriptor cfd) { + // CompoundConfiguration will look for keys in reverse order of addition, so we'd + // add global config first, then table and cf overrides, then cf metadata. + return new CompoundConfiguration().add(conf).addBytesMap(td.getValues()) + .addStringMap(cfd.getConfiguration()).addBytesMap(cfd.getValues()); + } + public static List toStoreFileInfo(Collection storefiles) { return storefiles.stream().map(HStoreFile::getFileInfo).collect(Collectors.toList()); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/FileBasedStoreFileTracker.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/FileBasedStoreFileTracker.java index c370b87c1154..4da7911bdded 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/FileBasedStoreFileTracker.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/FileBasedStoreFileTracker.java @@ -56,7 +56,14 @@ class FileBasedStoreFileTracker extends StoreFileTrackerBase { public FileBasedStoreFileTracker(Configuration conf, boolean isPrimaryReplica, StoreContext ctx) { super(conf, isPrimaryReplica, ctx); - backedFile = new StoreFileListFile(ctx); + //CreateTableProcedure needs to instantiate the configured SFT impl, in order to update table + //descriptors with the SFT impl specific configs. By the time this happens, the table has no + //regions nor stores yet, so it can't create a proper StoreContext. + if (ctx != null) { + backedFile = new StoreFileListFile(ctx); + } else { + backedFile = null; + } } @Override diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/MigrationStoreFileTracker.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/MigrationStoreFileTracker.java index 6b81cf091246..230c1ec1b7a8 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/MigrationStoreFileTracker.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/MigrationStoreFileTracker.java @@ -89,18 +89,6 @@ void set(List files) { "Should not call this method on " + getClass().getSimpleName()); } - @Override - public void persistConfiguration(TableDescriptorBuilder builder) { - super.persistConfiguration(builder); - TableDescriptor desc = builder.build(); - if (StringUtils.isEmpty(desc.getValue(SRC_IMPL))) { - builder.setValue(SRC_IMPL, src.getTrackerName()); - } - if (StringUtils.isEmpty(desc.getValue(DST_IMPL))) { - builder.setValue(DST_IMPL, dst.getTrackerName()); - } - } - static Class getSrcTrackerClass(Configuration conf) { return StoreFileTrackerFactory.getStoreFileTrackerClassForMigration(conf, SRC_IMPL); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTracker.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTracker.java index 59fe7ef52f96..fd8f7c99092d 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTracker.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTracker.java @@ -21,6 +21,7 @@ import java.util.Collection; import java.util.List; +import org.apache.hadoop.hbase.client.TableDescriptor; import org.apache.hadoop.hbase.client.TableDescriptorBuilder; import org.apache.hadoop.hbase.regionserver.CreateStoreFileWriterParams; import org.apache.hadoop.hbase.regionserver.StoreFileInfo; @@ -75,7 +76,7 @@ void replace(Collection compactedFiles, Collection StoreFileWriter createWriter(CreateStoreFileWriterParams params) throws IOException; /** - * Saves StoreFileTracker implementations specific configurations into the table descriptors. + * Adds StoreFileTracker implementations specific configurations into the table descriptor. *

* This is used to avoid accidentally data loss when changing the cluster level store file tracker * implementation, and also possible misconfiguration between master and region servers. @@ -83,5 +84,5 @@ void replace(Collection compactedFiles, Collection * See HBASE-26246 for more details. * @param builder The table descriptor builder for the given table. */ - void persistConfiguration(TableDescriptorBuilder builder); + TableDescriptorBuilder updateWithTrackerConfigs(TableDescriptorBuilder builder); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerBase.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerBase.java index 8a980b6b6ad2..23d56a51701c 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerBase.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerBase.java @@ -33,7 +33,6 @@ import org.apache.hadoop.hbase.io.hfile.HFile; import org.apache.hadoop.hbase.io.hfile.HFileContext; import org.apache.hadoop.hbase.io.hfile.HFileContextBuilder; -import org.apache.hadoop.hbase.procedure2.util.StringUtils; import org.apache.hadoop.hbase.regionserver.CreateStoreFileWriterParams; import org.apache.hadoop.hbase.regionserver.StoreContext; import org.apache.hadoop.hbase.regionserver.StoreFileInfo; @@ -84,11 +83,9 @@ public final void replace(Collection compactedFiles, } @Override - public void persistConfiguration(TableDescriptorBuilder builder) { - TableDescriptor desc = builder.build(); - if (StringUtils.isEmpty(desc.getValue(TRACKER_IMPL))) { - builder.setValue(TRACKER_IMPL, getTrackerName()); - } + public TableDescriptorBuilder updateWithTrackerConfigs(TableDescriptorBuilder builder) { + builder.setValue(TRACKER_IMPL, getTrackerName()); + return builder; } protected final String getTrackerName() { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java index 651990615f1f..a08ea724949e 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java @@ -19,6 +19,8 @@ import java.util.Collections; import java.util.HashMap; import java.util.Map; + +import org.apache.commons.lang3.StringUtils; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.CompoundConfiguration; import org.apache.hadoop.hbase.DoNotRetryIOException; @@ -27,6 +29,7 @@ import org.apache.hadoop.hbase.client.TableDescriptorBuilder; import org.apache.hadoop.hbase.regionserver.HRegionFileSystem; import org.apache.hadoop.hbase.regionserver.StoreContext; +import org.apache.hadoop.hbase.regionserver.StoreUtils; import org.apache.hadoop.hbase.util.ReflectionUtils; import org.apache.yetus.audience.InterfaceAudience; import org.slf4j.Logger; @@ -115,16 +118,13 @@ public static StoreFileTracker create(Configuration conf, TableDescriptor td, ColumnFamilyDescriptor cfd, HRegionFileSystem regionFs) { StoreContext ctx = StoreContext.getBuilder().withColumnFamilyDescriptor(cfd).withRegionFileSystem(regionFs) - .withFamilyStoreDirectoryPath(regionFs.getStoreDir(cfd.getNameAsString())) - .withFamilyStoreDirectoryPath(regionFs.getStoreDir(cfd.getNameAsString())) - .build(); + .withFamilyStoreDirectoryPath(regionFs.getStoreDir(cfd.getNameAsString())).build(); return StoreFileTrackerFactory.create(mergeConfigurations(conf, td, cfd), true, ctx); } private static Configuration mergeConfigurations(Configuration global, TableDescriptor table, ColumnFamilyDescriptor family) { - return new CompoundConfiguration().add(global).addBytesMap(table.getValues()) - .addStringMap(family.getConfiguration()).addBytesMap(family.getValues()); + return StoreUtils.createStoreConfiguration(global, table, family); } static Class @@ -161,12 +161,18 @@ static StoreFileTrackerBase createForMigration(Configuration conf, String config return ReflectionUtils.newInstance(tracker, conf, isPrimaryReplica, ctx); } - public static void persistTrackerConfig(Configuration conf, TableDescriptorBuilder builder) { - TableDescriptor tableDescriptor = builder.build(); - ColumnFamilyDescriptor cfDesc = tableDescriptor.getColumnFamilies()[0]; - StoreContext context = StoreContext.getBuilder().withColumnFamilyDescriptor(cfDesc).build(); - StoreFileTracker tracker = StoreFileTrackerFactory.create(conf, true, context); - tracker.persistConfiguration(builder); + public static TableDescriptor updateWithTrackerConfigs(Configuration conf, + TableDescriptor descriptor) { + //CreateTableProcedure needs to instantiate the configured SFT impl, in order to update table + //descriptors with the SFT impl specific configs. By the time this happens, the table has no + //regions nor stores yet, so it can't create a proper StoreContext. + if (StringUtils.isEmpty(descriptor.getValue(TRACKER_IMPL))) { + StoreFileTracker tracker = + StoreFileTrackerFactory.create(conf, true, null); + TableDescriptorBuilder builder = TableDescriptorBuilder.newBuilder(descriptor); + return tracker.updateWithTrackerConfigs(builder).build(); + } + return descriptor; } // should not use MigrationStoreFileTracker for new family diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestCreateTableProcedure.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestCreateTableProcedure.java index f432c8060d3d..51ea9f58248a 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestCreateTableProcedure.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestCreateTableProcedure.java @@ -39,6 +39,7 @@ import org.apache.hadoop.hbase.procedure2.Procedure; import org.apache.hadoop.hbase.procedure2.ProcedureExecutor; import org.apache.hadoop.hbase.procedure2.ProcedureTestingUtility; +import org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory; import org.apache.hadoop.hbase.regionserver.storefiletracker.TestStoreFileTracker; import org.apache.hadoop.hbase.testclassification.MasterTests; import org.apache.hadoop.hbase.testclassification.MediumTests; @@ -105,6 +106,21 @@ public void testCreateWithTrackImpl() throws Exception { assertEquals(trackerName, htd.getValue(TRACKER_IMPL)); } + @Test + public void testCreateWithFileBasedStoreTrackerImpl() throws Exception { + ProcedureExecutor procExec = getMasterProcedureExecutor(); + procExec.getEnvironment().getMasterConfiguration().set(StoreFileTrackerFactory.TRACKER_IMPL, + StoreFileTrackerFactory.Trackers.FILE.name()); + final TableName tableName = TableName.valueOf(name.getMethodName()); + TableDescriptor htd = MasterProcedureTestingUtility.createHTD(tableName, F1); + RegionInfo[] regions = ModifyRegionUtils.createRegionInfos(htd, null); + long procId = ProcedureTestingUtility.submitAndWait(procExec, + new CreateTableProcedure(procExec.getEnvironment(), htd, regions)); + ProcedureTestingUtility.assertProcNotFailed(procExec.getResult(procId)); + htd = getMaster().getTableDescriptors().get(tableName); + assertEquals(StoreFileTrackerFactory.Trackers.FILE.name(), htd.getValue(TRACKER_IMPL)); + } + @Test public void testCreateWithoutColumnFamily() throws Exception { final ProcedureExecutor procExec = getMasterProcedureExecutor(); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestChangeStoreFileTracker.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestChangeStoreFileTracker.java index 110f896df8b8..85fbec86652e 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestChangeStoreFileTracker.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestChangeStoreFileTracker.java @@ -65,7 +65,7 @@ public static void setUp() throws Exception { } @AfterClass - public static void tearDown() throws IOException { + public static void tearDown() throws Exception { UTIL.shutdownMiniCluster(); } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestStoreFileTracker.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestStoreFileTracker.java index 1dc9c4e53fc5..b30ca47772cb 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestStoreFileTracker.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestStoreFileTracker.java @@ -40,7 +40,7 @@ public class TestStoreFileTracker extends DefaultStoreFileTracker { public TestStoreFileTracker(Configuration conf, boolean isPrimaryReplica, StoreContext ctx) { super(conf, isPrimaryReplica, ctx); - if (ctx.getRegionFileSystem() != null) { + if (ctx != null && ctx.getRegionFileSystem() != null) { this.storeId = ctx.getRegionInfo().getEncodedName() + "-" + ctx.getFamily().getNameAsString(); LOG.info("created storeId: {}", storeId); trackedFiles.computeIfAbsent(storeId, v -> new ArrayList<>());