From 3886daecea79d03dd29d476e07d3924b4583d92a Mon Sep 17 00:00:00 2001 From: Bryan Beaudreault Date: Wed, 6 Dec 2023 08:33:20 -0500 Subject: [PATCH 1/7] HBASE-28216 HDFS erasure coding support for table data dirs --- .../hadoop/hbase/client/TableDescriptor.java | 9 + .../hbase/client/TableDescriptorBuilder.java | 34 ++++ .../server/master/MasterProcedure.proto | 1 + .../hadoop/hbase/fs/ErasureCodingUtils.java | 93 +++++++++ .../procedure/ModifyTableProcedure.java | 36 +++- .../CompactedHFilesDischarger.java | 2 +- .../TestModifyTableErasureCodingPolicy.java | 185 ++++++++++++++++++ 7 files changed, 357 insertions(+), 3 deletions(-) create mode 100644 hbase-server/src/main/java/org/apache/hadoop/hbase/fs/ErasureCodingUtils.java create mode 100644 hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableErasureCodingPolicy.java diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptor.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptor.java index 1c91819ac4b9..817f9e2d4b1b 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptor.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptor.java @@ -266,6 +266,15 @@ public interface TableDescriptor { */ boolean isReadOnly(); + /** + * The HDFS erasure coding policy for a table. This will be set on the data dir of the table, and + * is an alternative to normal replication which takes less space at the cost of locality. + * @return the current policy, or null if undefined + */ + default String getErasureCodingPolicy() { + return null; + } + /** * Returns Name of this table and then a map of all of the column family descriptors (with only * the non-default column family attributes) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java index 43ca935ffa17..83bec1285545 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java @@ -143,6 +143,13 @@ public class TableDescriptorBuilder { private static final Bytes REGION_MEMSTORE_REPLICATION_KEY = new Bytes(Bytes.toBytes(REGION_MEMSTORE_REPLICATION)); + /** + * If non-null, the HDFS erasure coding policy to set on the data dir of the table + */ + public static String ERASURE_CODING_POLICY = "ERASURE_CODING_POLICY"; + private static final Bytes ERASURE_CODING_POLICY_KEY = + new Bytes(Bytes.toBytes(ERASURE_CODING_POLICY)); + /** * Used by shell/rest interface to access this metadata attribute which denotes if the table * should be treated by region normalizer. @@ -490,6 +497,11 @@ public TableDescriptorBuilder setReadOnly(final boolean readOnly) { return this; } + public TableDescriptorBuilder setErasureCodingPolicy(String policy) { + desc.setErasureCodingPolicy(policy); + return this; + } + public TableDescriptorBuilder setRegionMemStoreReplication(boolean memstoreReplication) { desc.setRegionMemStoreReplication(memstoreReplication); return this; @@ -748,6 +760,28 @@ public ModifyableTableDescriptor setReadOnly(final boolean readOnly) { return setValue(READONLY_KEY, Boolean.toString(readOnly)); } + /** + * The HDFS erasure coding policy for a table. This will be set on the data dir of the table, + * and is an alternative to normal replication which takes less space at the cost of locality. + * @return the current policy, or null if undefined + */ + @Override + public String getErasureCodingPolicy() { + return getValue(ERASURE_CODING_POLICY); + } + + /** + * Sets the HDFS erasure coding policy for the table. This will be propagated to HDFS for the + * data dir of the table. Erasure coding is an alternative to normal replication which takes + * less space at the cost of locality. The policy must be available and enabled on the hdfs + * cluster before being set. + * @param policy the policy to set + * @return the modifyable TD + */ + public ModifyableTableDescriptor setErasureCodingPolicy(String policy) { + return setValue(ERASURE_CODING_POLICY_KEY, policy); + } + /** * Check if the compaction enable flag of the table is true. If flag is false then no * minor/major compactions will be done in real. diff --git a/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto b/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto index 6c5501c9d0d6..6650fd77f46e 100644 --- a/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto +++ b/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto @@ -74,6 +74,7 @@ enum ModifyTableState { MODIFY_TABLE_REOPEN_ALL_REGIONS = 7; MODIFY_TABLE_CLOSE_EXCESS_REPLICAS = 8; MODIFY_TABLE_ASSIGN_NEW_REPLICAS = 9; + MODIFY_TABLE_SYNC_ERASURE_CODING_POLICY = 10; } message ModifyTableStateData { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/fs/ErasureCodingUtils.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/fs/ErasureCodingUtils.java new file mode 100644 index 000000000000..69c02004c540 --- /dev/null +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/fs/ErasureCodingUtils.java @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.fs; + +import java.io.IOException; +import java.util.Collection; +import java.util.stream.Collectors; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.HBaseIOException; +import org.apache.hadoop.hdfs.DistributedFileSystem; +import org.apache.hadoop.hdfs.protocol.ErasureCodingPolicyInfo; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@InterfaceAudience.Private +public class ErasureCodingUtils { + + private static final Logger LOG = LoggerFactory.getLogger(ErasureCodingUtils.class); + + /** + * Runs checks against the FileSystem, verifying that HDFS is supported and the policy is + * available and enabled. + */ + public static void checkAvailable(FileSystem fs, String policy) throws HBaseIOException { + DistributedFileSystem dfs = getDfs(fs); + + Collection policies; + try { + policies = dfs.getAllErasureCodingPolicies(); + } catch (IOException e) { + throw new HBaseIOException("Failed to check for Erasure Coding policy: " + policy, e); + } + + for (ErasureCodingPolicyInfo policyInfo : policies) { + if (policyInfo.getPolicy().getName().equals(policy)) { + if (!policyInfo.isEnabled()) { + throw new HBaseIOException("Cannot set Erasure Coding policy: " + policy + + ". The policy must be enabled, but has state " + policyInfo.getState()); + } + return; + } + } + + throw new HBaseIOException( + "Cannot set Erasure Coding policy: " + policy + ". Policy not found. Available policies are: " + + policies.stream().map(p -> p.getPolicy().getName()).collect(Collectors.joining(", "))); + } + + /** + * Sets the EC policy on the path + */ + public static void setPolicy(FileSystem fs, Path path, String policy) throws IOException { + getDfs(fs).setErasureCodingPolicy(path, policy); + } + + /** + * Unsets any EC policy specified on the path. + */ + public static void unsetPolicy(FileSystem fs, Path path) throws IOException { + DistributedFileSystem dfs = getDfs(fs); + if (dfs.getErasureCodingPolicy(path) == null) { + LOG.warn("No EC policy set for path {}, nothing to unset", path); + return; + } + dfs.unsetErasureCodingPolicy(path); + } + + private static DistributedFileSystem getDfs(FileSystem fs) throws HBaseIOException { + if (!(fs instanceof DistributedFileSystem)) { + throw new HBaseIOException( + "Cannot manage Erasure Coding policy. Erasure Coding is only available on HDFS, but fs is " + + fs.getClass().getSimpleName()); + } + return (DistributedFileSystem) fs; + } +} diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java index f7314349ee2c..a79a6389b9fb 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java @@ -27,11 +27,14 @@ import java.util.Collections; import java.util.HashSet; import java.util.List; +import java.util.Objects; import java.util.Set; import java.util.function.Supplier; import java.util.stream.Collectors; import java.util.stream.IntStream; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.ConcurrentTableModificationException; import org.apache.hadoop.hbase.DoNotRetryIOException; import org.apache.hadoop.hbase.HBaseIOException; @@ -42,12 +45,14 @@ 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.fs.ErasureCodingUtils; import org.apache.hadoop.hbase.master.MasterCoprocessorHost; import org.apache.hadoop.hbase.master.zksyncer.MetaLocationSyncer; import org.apache.hadoop.hbase.procedure2.ProcedureStateSerializer; import org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerValidationUtils; import org.apache.hadoop.hbase.rsgroup.RSGroupInfo; import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.util.CommonFSUtils; import org.apache.yetus.audience.InterfaceAudience; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -114,6 +119,12 @@ protected void preflightChecks(MasterProcedureEnv env, Boolean enabled) throws H } } } + + String policy = modifiedTableDescriptor.getErasureCodingPolicy(); + if (policy != null) { + ErasureCodingUtils.checkAvailable(env.getMasterFileSystem().getFileSystem(), policy); + } + if (!reopenRegions) { if (this.unmodifiedTableDescriptor == null) { throw new HBaseIOException( @@ -220,7 +231,7 @@ protected Flow executeFromState(final MasterProcedureEnv env, final ModifyTableS if (reopenRegions) { setNextState(ModifyTableState.MODIFY_TABLE_REOPEN_ALL_REGIONS); } else { - return Flow.NO_MORE_STATE; + setNextState(ModifyTableState.MODIFY_TABLE_SYNC_ERASURE_CODING_POLICY); } break; case MODIFY_TABLE_REOPEN_ALL_REGIONS: @@ -246,11 +257,15 @@ protected Flow executeFromState(final MasterProcedureEnv env, final ModifyTableS if (deleteColumnFamilyInModify) { setNextState(ModifyTableState.MODIFY_TABLE_DELETE_FS_LAYOUT); } else { - return Flow.NO_MORE_STATE; + setNextState(ModifyTableState.MODIFY_TABLE_SYNC_ERASURE_CODING_POLICY); } break; case MODIFY_TABLE_DELETE_FS_LAYOUT: deleteFromFs(env, unmodifiedTableDescriptor, modifiedTableDescriptor); + setNextState(ModifyTableState.MODIFY_TABLE_SYNC_ERASURE_CODING_POLICY); + break; + case MODIFY_TABLE_SYNC_ERASURE_CODING_POLICY: + syncErasureCodingPolicy(env); return Flow.NO_MORE_STATE; default: throw new UnsupportedOperationException("unhandled state=" + state); @@ -512,6 +527,23 @@ private void closeExcessReplicasIfNeeded(MasterProcedureEnv env) { .createUnassignProceduresForClosingExcessRegionReplicas(getTableName(), newReplicaCount)); } + private void syncErasureCodingPolicy(MasterProcedureEnv env) throws IOException { + String oldPolicy = unmodifiedTableDescriptor.getErasureCodingPolicy(); + String newPolicy = modifiedTableDescriptor.getErasureCodingPolicy(); + if (Objects.equals(oldPolicy, newPolicy)) { + return; + } + + FileSystem fileSystem = env.getMasterFileSystem().getFileSystem(); + Path tableDir = CommonFSUtils.getTableDir(env.getMasterFileSystem().getRootDir(), + unmodifiedTableDescriptor.getTableName()); + if (newPolicy == null) { + ErasureCodingUtils.unsetPolicy(fileSystem, tableDir); + } else { + ErasureCodingUtils.setPolicy(fileSystem, tableDir, newPolicy); + } + } + /** * Action after modifying table. * @param env MasterProcedureEnv diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactedHFilesDischarger.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactedHFilesDischarger.java index 984b9e09a5af..2f4eaa085378 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactedHFilesDischarger.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactedHFilesDischarger.java @@ -70,7 +70,7 @@ public CompactedHFilesDischarger(final int period, final Stoppable stopper, * no-executor before you call run. * @return The old setting for useExecutor */ - boolean setUseExecutor(final boolean useExecutor) { + public boolean setUseExecutor(final boolean useExecutor) { boolean oldSetting = this.useExecutor; this.useExecutor = useExecutor; return oldSetting; diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableErasureCodingPolicy.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableErasureCodingPolicy.java new file mode 100644 index 000000000000..bd0c689c3a30 --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableErasureCodingPolicy.java @@ -0,0 +1,185 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.master.procedure; + +import static org.hamcrest.MatcherAssert.*; +import static org.hamcrest.Matchers.*; +import static org.junit.Assert.assertThrows; + +import java.io.IOException; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.LocatedFileStatus; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.RemoteIterator; +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.HBaseIOException; +import org.apache.hadoop.hbase.HBaseTestingUtil; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.Admin; +import org.apache.hadoop.hbase.client.Table; +import org.apache.hadoop.hbase.client.TableDescriptor; +import org.apache.hadoop.hbase.client.TableDescriptorBuilder; +import org.apache.hadoop.hbase.io.hfile.HFile; +import org.apache.hadoop.hbase.regionserver.CompactedHFilesDischarger; +import org.apache.hadoop.hbase.regionserver.HRegion; +import org.apache.hadoop.hbase.testclassification.MasterTests; +import org.apache.hadoop.hbase.testclassification.MediumTests; +import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.util.CommonFSUtils; +import org.apache.hadoop.hbase.util.JVMClusterUtil; +import org.apache.hadoop.hdfs.DistributedFileSystem; +import org.apache.hadoop.hdfs.protocol.ErasureCodingPolicy; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +@Category({ MasterTests.class, MediumTests.class }) +public class TestModifyTableErasureCodingPolicy { + + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestModifyTableErasureCodingPolicy.class); + + private static final HBaseTestingUtil UTIL = new HBaseTestingUtil(); + private static final TableName TABLE = TableName.valueOf("foo"); + private static final byte[] FAMILY = Bytes.toBytes("a"); + + @BeforeClass + public static void beforeClass() throws Exception { + UTIL.startMiniDFSCluster(6); // 6 necessary for RS-6-3-1024k + UTIL.startMiniCluster(1); + Table table = UTIL.createTable(TABLE, FAMILY); + UTIL.loadTable(table, FAMILY); + UTIL.flush(); + } + + @AfterClass + public static void afterClass() throws Exception { + UTIL.shutdownMiniCluster(); + UTIL.shutdownMiniDFSCluster(); + } + + @Test + public void itValidatesPolicyName() { + HBaseIOException thrown = assertThrows(HBaseIOException.class, () -> { + try (Admin admin = UTIL.getAdmin()) { + TableDescriptor desc = UTIL.getAdmin().getDescriptor(TABLE); + admin.modifyTable( + TableDescriptorBuilder.newBuilder(desc).setErasureCodingPolicy("foo").build()); + } + }); + assertThat(thrown.getMessage(), + containsString("Cannot set Erasure Coding policy: foo. Policy not found")); + + thrown = assertThrows(HBaseIOException.class, () -> { + try (Admin admin = UTIL.getAdmin()) { + TableDescriptor desc = UTIL.getAdmin().getDescriptor(TABLE); + admin.modifyTable( + TableDescriptorBuilder.newBuilder(desc).setErasureCodingPolicy("RS-10-4-1024k").build()); + } + }); + assertThat(thrown.getMessage(), containsString( + "Cannot set Erasure Coding policy: RS-10-4-1024k. The policy must be enabled")); + } + + @Test + public void testErasureCodingSync() throws IOException, InterruptedException { + try (Admin admin = UTIL.getAdmin()) { + Path rootDir = CommonFSUtils.getRootDir(UTIL.getConfiguration()); + Path tableDir = CommonFSUtils.getTableDir(rootDir, TABLE); + DistributedFileSystem dfs = (DistributedFileSystem) FileSystem.get(UTIL.getConfiguration()); + + // start off without EC + checkRegionDirAndFilePolicies(dfs, tableDir, null, null); + + // add EC + TableDescriptor desc = UTIL.getAdmin().getDescriptor(TABLE); + admin.modifyTable( + TableDescriptorBuilder.newBuilder(desc).setErasureCodingPolicy("RS-6-3-1024k").build()); + + // check dirs, but files should not be changed yet + checkRegionDirAndFilePolicies(dfs, tableDir, "RS-6-3-1024k", null); + + // compact to rewrite files with EC, then run discharger to get rid of the old non-EC files + UTIL.compact(TABLE, true); + for (JVMClusterUtil.RegionServerThread regionserver : UTIL.getHBaseCluster() + .getLiveRegionServerThreads()) { + CompactedHFilesDischarger chore = + regionserver.getRegionServer().getCompactedHFilesDischarger(); + chore.setUseExecutor(false); + chore.chore(); + } + + // expect both dirs and files to be EC now + checkRegionDirAndFilePolicies(dfs, tableDir, "RS-6-3-1024k", "RS-6-3-1024k"); + + // remove EC now + admin.modifyTable(desc); + + // dirs should no longer be EC, but old EC files remain + checkRegionDirAndFilePolicies(dfs, tableDir, null, "RS-6-3-1024k"); + + // compact to rewrite EC files without EC, then run discharger to get rid of the old EC files + UTIL.compact(TABLE, true); + for (JVMClusterUtil.RegionServerThread regionserver : UTIL.getHBaseCluster() + .getLiveRegionServerThreads()) { + CompactedHFilesDischarger chore = + regionserver.getRegionServer().getCompactedHFilesDischarger(); + chore.setUseExecutor(false); + chore.chore(); + } + + checkRegionDirAndFilePolicies(dfs, tableDir, null, null); + } + } + + private void checkRegionDirAndFilePolicies(DistributedFileSystem dfs, Path tableDir, + String expectedDirPolicy, String expectedFilePolicy) throws IOException { + checkPolicy(dfs, tableDir, expectedDirPolicy); + + int filesMatched = 0; + for (HRegion region : UTIL.getHBaseCluster().getRegions(TABLE)) { + Path regionDir = new Path(tableDir, region.getRegionInfo().getEncodedName()); + checkPolicy(dfs, regionDir, expectedDirPolicy); + RemoteIterator itr = dfs.listFiles(regionDir, true); + while (itr.hasNext()) { + LocatedFileStatus fileStatus = itr.next(); + Path path = fileStatus.getPath(); + if (!HFile.isHFileFormat(dfs, path)) { + continue; + } + filesMatched++; + checkPolicy(dfs, path, expectedFilePolicy); + } + } + assertThat(filesMatched, greaterThan(0)); + } + + private void checkPolicy(DistributedFileSystem dfs, Path path, String expectedPolicy) + throws IOException { + ErasureCodingPolicy policy = dfs.getErasureCodingPolicy(path); + if (expectedPolicy == null) { + assertThat(policy, nullValue()); + } else { + assertThat("policy for " + path, policy, notNullValue()); + assertThat("policy for " + path, policy.getName(), equalTo(expectedPolicy)); + } + } +} From d84cac8104cb7bee82100c2ec4586fdf00918b20 Mon Sep 17 00:00:00 2001 From: Bryan Beaudreault Date: Wed, 13 Dec 2023 19:41:04 -0500 Subject: [PATCH 2/7] precommit fixes --- .../hadoop/hbase/client/TableDescriptorBuilder.java | 2 +- .../org/apache/hadoop/hbase/fs/ErasureCodingUtils.java | 5 ++++- .../procedure/TestModifyTableErasureCodingPolicy.java | 8 ++++++-- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java index 83bec1285545..6145f5602df6 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java @@ -146,7 +146,7 @@ public class TableDescriptorBuilder { /** * If non-null, the HDFS erasure coding policy to set on the data dir of the table */ - public static String ERASURE_CODING_POLICY = "ERASURE_CODING_POLICY"; + public static final String ERASURE_CODING_POLICY = "ERASURE_CODING_POLICY"; private static final Bytes ERASURE_CODING_POLICY_KEY = new Bytes(Bytes.toBytes(ERASURE_CODING_POLICY)); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/fs/ErasureCodingUtils.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/fs/ErasureCodingUtils.java index 69c02004c540..7c337afda74c 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/fs/ErasureCodingUtils.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/fs/ErasureCodingUtils.java @@ -30,7 +30,10 @@ import org.slf4j.LoggerFactory; @InterfaceAudience.Private -public class ErasureCodingUtils { +public final class ErasureCodingUtils { + + private ErasureCodingUtils() { + } private static final Logger LOG = LoggerFactory.getLogger(ErasureCodingUtils.class); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableErasureCodingPolicy.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableErasureCodingPolicy.java index bd0c689c3a30..3170e4f8a71c 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableErasureCodingPolicy.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableErasureCodingPolicy.java @@ -17,8 +17,12 @@ */ package org.apache.hadoop.hbase.master.procedure; -import static org.hamcrest.MatcherAssert.*; -import static org.hamcrest.Matchers.*; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.greaterThan; +import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; import static org.junit.Assert.assertThrows; import java.io.IOException; From 3f5992f0182fcd1566c78758729c1db10b916a33 Mon Sep 17 00:00:00 2001 From: Bryan Beaudreault Date: Sat, 16 Dec 2023 21:42:34 -0500 Subject: [PATCH 3/7] add shell support, add create support and tests --- .../hbase/client/TableDescriptorBuilder.java | 4 +- .../client/TestTableDescriptorBuilder.java | 4 +- .../server/master/MasterProcedure.proto | 1 + .../procedure/CreateTableProcedure.java | 15 ++++ ...> TestManageTableErasureCodingPolicy.java} | 88 ++++++++++++++----- hbase-shell/src/main/ruby/hbase/admin.rb | 1 + 6 files changed, 87 insertions(+), 26 deletions(-) rename hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/{TestModifyTableErasureCodingPolicy.java => TestManageTableErasureCodingPolicy.java} (64%) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java index 6145f5602df6..7cab90c7e378 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java @@ -150,6 +150,7 @@ public class TableDescriptorBuilder { private static final Bytes ERASURE_CODING_POLICY_KEY = new Bytes(Bytes.toBytes(ERASURE_CODING_POLICY)); + private static final String DEFAULT_ERASURE_CODING_POLICY = null; /** * Used by shell/rest interface to access this metadata attribute which denotes if the table * should be treated by region normalizer. @@ -233,6 +234,7 @@ public class TableDescriptorBuilder { DEFAULT_VALUES.put(DURABILITY, DEFAULT_DURABLITY.name()); // use the enum name DEFAULT_VALUES.put(REGION_REPLICATION, String.valueOf(DEFAULT_REGION_REPLICATION)); DEFAULT_VALUES.put(PRIORITY, String.valueOf(DEFAULT_PRIORITY)); + DEFAULT_VALUES.put(ERASURE_CODING_POLICY, String.valueOf(DEFAULT_ERASURE_CODING_POLICY)); DEFAULT_VALUES.keySet().stream().map(s -> new Bytes(Bytes.toBytes(s))) .forEach(RESERVED_KEYWORDS::add); RESERVED_KEYWORDS.add(IS_META_KEY); @@ -775,7 +777,7 @@ public String getErasureCodingPolicy() { * data dir of the table. Erasure coding is an alternative to normal replication which takes * less space at the cost of locality. The policy must be available and enabled on the hdfs * cluster before being set. - * @param policy the policy to set + * @param policy the policy to set, or null to disable erasure coding * @return the modifyable TD */ public ModifyableTableDescriptor setErasureCodingPolicy(String policy) { diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestTableDescriptorBuilder.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestTableDescriptorBuilder.java index 81db193a3042..53f33845ef7d 100644 --- a/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestTableDescriptorBuilder.java +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestTableDescriptorBuilder.java @@ -362,10 +362,10 @@ public void testStringCustomizedValues() throws HBaseException { htd.toStringCustomizedValues()); htd = TableDescriptorBuilder.newBuilder(htd).setMaxFileSize("10737942528") - .setMemStoreFlushSize("256MB").build(); + .setMemStoreFlushSize("256MB").setErasureCodingPolicy("RS-6-3-1024k").build(); assertEquals( "'testStringCustomizedValues', " + "{TABLE_ATTRIBUTES => {DURABILITY => 'ASYNC_WAL', " - + "MAX_FILESIZE => '10737942528 B (10GB 512KB)', " + + "ERASURE_CODING_POLICY => 'RS-6-3-1024k', MAX_FILESIZE => '10737942528 B (10GB 512KB)', " + "MEMSTORE_FLUSHSIZE => '268435456 B (256MB)'}}, " + "{NAME => 'cf', BLOCKSIZE => '131072 B (128KB)'}", htd.toStringCustomizedValues()); diff --git a/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto b/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto index 6650fd77f46e..24919977da6e 100644 --- a/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto +++ b/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto @@ -56,6 +56,7 @@ enum CreateTableState { CREATE_TABLE_ASSIGN_REGIONS = 4; CREATE_TABLE_UPDATE_DESC_CACHE = 5; CREATE_TABLE_POST_OPERATION = 6; + CREATE_TABLE_SYNC_ERASURE_CODING_POLICY = 7; } message CreateTableStateData { 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 34e1f3d5bfc3..53896c31bd50 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,6 +31,7 @@ import org.apache.hadoop.hbase.client.RegionReplicaUtil; import org.apache.hadoop.hbase.client.TableDescriptor; import org.apache.hadoop.hbase.client.TableState; +import org.apache.hadoop.hbase.fs.ErasureCodingUtils; import org.apache.hadoop.hbase.master.MasterCoprocessorHost; import org.apache.hadoop.hbase.master.MasterFileSystem; import org.apache.hadoop.hbase.procedure2.ProcedureStateSerializer; @@ -98,6 +99,15 @@ protected Flow executeFromState(final MasterProcedureEnv env, final CreateTableS DeleteTableProcedure.deleteFromFs(env, getTableName(), newRegions, true); newRegions = createFsLayout(env, tableDescriptor, newRegions); env.getMasterServices().getTableDescriptors().update(tableDescriptor, true); + setNextState(CreateTableState.CREATE_TABLE_SYNC_ERASURE_CODING_POLICY); + break; + case CREATE_TABLE_SYNC_ERASURE_CODING_POLICY: + if (tableDescriptor.getErasureCodingPolicy() != null) { + final Path tableDir = CommonFSUtils.getTableDir(env.getMasterFileSystem().getRootDir(), + tableDescriptor.getTableName()); + ErasureCodingUtils.setPolicy(env.getMasterFileSystem().getFileSystem(), tableDir, + tableDescriptor.getErasureCodingPolicy()); + } setNextState(CreateTableState.CREATE_TABLE_ADD_TO_META); break; case CREATE_TABLE_ADD_TO_META: @@ -273,6 +283,11 @@ private boolean prepareCreate(final MasterProcedureEnv env) throws IOException { StoreFileTrackerValidationUtils.checkForCreateTable(env.getMasterConfiguration(), tableDescriptor); + if (tableDescriptor.getErasureCodingPolicy() != null) { + ErasureCodingUtils.checkAvailable(env.getMasterFileSystem().getFileSystem(), + tableDescriptor.getErasureCodingPolicy()); + } + return true; } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableErasureCodingPolicy.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestManageTableErasureCodingPolicy.java similarity index 64% rename from hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableErasureCodingPolicy.java rename to hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestManageTableErasureCodingPolicy.java index 3170e4f8a71c..1fae7948ac13 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableErasureCodingPolicy.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestManageTableErasureCodingPolicy.java @@ -35,6 +35,7 @@ import org.apache.hadoop.hbase.HBaseTestingUtil; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.client.Admin; +import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder; import org.apache.hadoop.hbase.client.Table; import org.apache.hadoop.hbase.client.TableDescriptor; import org.apache.hadoop.hbase.client.TableDescriptorBuilder; @@ -55,21 +56,27 @@ import org.junit.experimental.categories.Category; @Category({ MasterTests.class, MediumTests.class }) -public class TestModifyTableErasureCodingPolicy { +public class TestManageTableErasureCodingPolicy { @ClassRule public static final HBaseClassTestRule CLASS_RULE = - HBaseClassTestRule.forClass(TestModifyTableErasureCodingPolicy.class); + HBaseClassTestRule.forClass(TestManageTableErasureCodingPolicy.class); private static final HBaseTestingUtil UTIL = new HBaseTestingUtil(); - private static final TableName TABLE = TableName.valueOf("foo"); private static final byte[] FAMILY = Bytes.toBytes("a"); + private static final TableName NON_EC_TABLE = TableName.valueOf("foo"); + private static final TableDescriptor NON_EC_TABLE_DESC = TableDescriptorBuilder + .newBuilder(NON_EC_TABLE).setColumnFamily(ColumnFamilyDescriptorBuilder.of(FAMILY)).build(); + private static final TableName EC_TABLE = TableName.valueOf("bar"); + private static final TableDescriptor EC_TABLE_DESC = + TableDescriptorBuilder.newBuilder(EC_TABLE).setErasureCodingPolicy("RS-6-3-1024k") + .setColumnFamily(ColumnFamilyDescriptorBuilder.of(FAMILY)).build(); @BeforeClass public static void beforeClass() throws Exception { UTIL.startMiniDFSCluster(6); // 6 necessary for RS-6-3-1024k UTIL.startMiniCluster(1); - Table table = UTIL.createTable(TABLE, FAMILY); + Table table = UTIL.createTable(NON_EC_TABLE_DESC, null); UTIL.loadTable(table, FAMILY); UTIL.flush(); } @@ -81,10 +88,31 @@ public static void afterClass() throws Exception { } @Test - public void itValidatesPolicyName() { + public void itValidatesPolicyNameForCreate() { HBaseIOException thrown = assertThrows(HBaseIOException.class, () -> { try (Admin admin = UTIL.getAdmin()) { - TableDescriptor desc = UTIL.getAdmin().getDescriptor(TABLE); + admin.createTable( + TableDescriptorBuilder.newBuilder(EC_TABLE_DESC).setErasureCodingPolicy("foo").build()); + } + }); + assertThat(thrown.getMessage(), + containsString("Cannot set Erasure Coding policy: foo. Policy not found")); + + thrown = assertThrows(HBaseIOException.class, () -> { + try (Admin admin = UTIL.getAdmin()) { + admin.createTable(TableDescriptorBuilder.newBuilder(EC_TABLE_DESC) + .setErasureCodingPolicy("RS-10-4-1024k").build()); + } + }); + assertThat(thrown.getMessage(), containsString( + "Cannot set Erasure Coding policy: RS-10-4-1024k. The policy must be enabled")); + } + + @Test + public void itValidatesPolicyNameForAlter() { + HBaseIOException thrown = assertThrows(HBaseIOException.class, () -> { + try (Admin admin = UTIL.getAdmin()) { + TableDescriptor desc = UTIL.getAdmin().getDescriptor(NON_EC_TABLE); admin.modifyTable( TableDescriptorBuilder.newBuilder(desc).setErasureCodingPolicy("foo").build()); } @@ -94,7 +122,7 @@ public void itValidatesPolicyName() { thrown = assertThrows(HBaseIOException.class, () -> { try (Admin admin = UTIL.getAdmin()) { - TableDescriptor desc = UTIL.getAdmin().getDescriptor(TABLE); + TableDescriptor desc = UTIL.getAdmin().getDescriptor(NON_EC_TABLE); admin.modifyTable( TableDescriptorBuilder.newBuilder(desc).setErasureCodingPolicy("RS-10-4-1024k").build()); } @@ -104,25 +132,37 @@ public void itValidatesPolicyName() { } @Test - public void testErasureCodingSync() throws IOException, InterruptedException { + public void testCreateTableErasureCodingSync() throws IOException { + try (Admin admin = UTIL.getAdmin(); Table table = UTIL.getConnection().getTable(EC_TABLE)) { + admin.createTable(EC_TABLE_DESC); + UTIL.loadTable(table, FAMILY); + UTIL.flush(EC_TABLE); + Path rootDir = CommonFSUtils.getRootDir(UTIL.getConfiguration()); + DistributedFileSystem dfs = (DistributedFileSystem) FileSystem.get(UTIL.getConfiguration()); + checkRegionDirAndFilePolicies(dfs, rootDir, EC_TABLE, "RS-6-3-1024k", "RS-6-3-1024k"); + } + } + + @Test + public void testModifyTableErasureCodingSync() throws IOException, InterruptedException { try (Admin admin = UTIL.getAdmin()) { Path rootDir = CommonFSUtils.getRootDir(UTIL.getConfiguration()); - Path tableDir = CommonFSUtils.getTableDir(rootDir, TABLE); DistributedFileSystem dfs = (DistributedFileSystem) FileSystem.get(UTIL.getConfiguration()); // start off without EC - checkRegionDirAndFilePolicies(dfs, tableDir, null, null); + checkRegionDirAndFilePolicies(dfs, rootDir, NON_EC_TABLE, null, null); // add EC - TableDescriptor desc = UTIL.getAdmin().getDescriptor(TABLE); - admin.modifyTable( - TableDescriptorBuilder.newBuilder(desc).setErasureCodingPolicy("RS-6-3-1024k").build()); + TableDescriptor desc = UTIL.getAdmin().getDescriptor(NON_EC_TABLE); + TableDescriptor newDesc = + TableDescriptorBuilder.newBuilder(desc).setErasureCodingPolicy("RS-6-3-1024k").build(); + admin.modifyTable(newDesc); // check dirs, but files should not be changed yet - checkRegionDirAndFilePolicies(dfs, tableDir, "RS-6-3-1024k", null); + checkRegionDirAndFilePolicies(dfs, rootDir, NON_EC_TABLE, "RS-6-3-1024k", null); // compact to rewrite files with EC, then run discharger to get rid of the old non-EC files - UTIL.compact(TABLE, true); + UTIL.compact(NON_EC_TABLE, true); for (JVMClusterUtil.RegionServerThread regionserver : UTIL.getHBaseCluster() .getLiveRegionServerThreads()) { CompactedHFilesDischarger chore = @@ -132,16 +172,17 @@ public void testErasureCodingSync() throws IOException, InterruptedException { } // expect both dirs and files to be EC now - checkRegionDirAndFilePolicies(dfs, tableDir, "RS-6-3-1024k", "RS-6-3-1024k"); + checkRegionDirAndFilePolicies(dfs, rootDir, NON_EC_TABLE, "RS-6-3-1024k", "RS-6-3-1024k"); + newDesc = TableDescriptorBuilder.newBuilder(newDesc).setErasureCodingPolicy(null).build(); // remove EC now - admin.modifyTable(desc); + admin.modifyTable(newDesc); // dirs should no longer be EC, but old EC files remain - checkRegionDirAndFilePolicies(dfs, tableDir, null, "RS-6-3-1024k"); + checkRegionDirAndFilePolicies(dfs, rootDir, NON_EC_TABLE, null, "RS-6-3-1024k"); // compact to rewrite EC files without EC, then run discharger to get rid of the old EC files - UTIL.compact(TABLE, true); + UTIL.compact(NON_EC_TABLE, true); for (JVMClusterUtil.RegionServerThread regionserver : UTIL.getHBaseCluster() .getLiveRegionServerThreads()) { CompactedHFilesDischarger chore = @@ -150,16 +191,17 @@ public void testErasureCodingSync() throws IOException, InterruptedException { chore.chore(); } - checkRegionDirAndFilePolicies(dfs, tableDir, null, null); + checkRegionDirAndFilePolicies(dfs, rootDir, NON_EC_TABLE, null, null); } } - private void checkRegionDirAndFilePolicies(DistributedFileSystem dfs, Path tableDir, - String expectedDirPolicy, String expectedFilePolicy) throws IOException { + private void checkRegionDirAndFilePolicies(DistributedFileSystem dfs, Path rootDir, + TableName testTable, String expectedDirPolicy, String expectedFilePolicy) throws IOException { + Path tableDir = CommonFSUtils.getTableDir(rootDir, testTable); checkPolicy(dfs, tableDir, expectedDirPolicy); int filesMatched = 0; - for (HRegion region : UTIL.getHBaseCluster().getRegions(TABLE)) { + for (HRegion region : UTIL.getHBaseCluster().getRegions(testTable)) { Path regionDir = new Path(tableDir, region.getRegionInfo().getEncodedName()); checkPolicy(dfs, regionDir, expectedDirPolicy); RemoteIterator itr = dfs.listFiles(regionDir, true); diff --git a/hbase-shell/src/main/ruby/hbase/admin.rb b/hbase-shell/src/main/ruby/hbase/admin.rb index 5c6d778bcf37..cf9d5dc65ca6 100644 --- a/hbase-shell/src/main/ruby/hbase/admin.rb +++ b/hbase-shell/src/main/ruby/hbase/admin.rb @@ -1580,6 +1580,7 @@ def list_locks # Parse arguments and update TableDescriptorBuilder accordingly # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity def update_tdb_from_arg(tdb, arg) + tdb.setErasureCodingPolicy(arg.delete(TableDescriptorBuilder::ERASURE_CODING_POLICY)) if arg.include?(TableDescriptorBuilder::ERASURE_CODING_POLICY) tdb.setMaxFileSize(arg.delete(TableDescriptorBuilder::MAX_FILESIZE)) if arg.include?(TableDescriptorBuilder::MAX_FILESIZE) tdb.setReadOnly(JBoolean.valueOf(arg.delete(TableDescriptorBuilder::READONLY))) if arg.include?(TableDescriptorBuilder::READONLY) tdb.setCompactionEnabled(JBoolean.valueOf(arg.delete(TableDescriptorBuilder::COMPACTION_ENABLED))) if arg.include?(TableDescriptorBuilder::COMPACTION_ENABLED) From a9c830457017fa9580b5f6b88ff29249785ed7b0 Mon Sep 17 00:00:00 2001 From: Bryan Beaudreault Date: Sat, 16 Dec 2023 22:08:45 -0500 Subject: [PATCH 4/7] shellt tests --- hbase-shell/src/test/ruby/hbase/admin_test.rb | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/hbase-shell/src/test/ruby/hbase/admin_test.rb b/hbase-shell/src/test/ruby/hbase/admin_test.rb index 4efcbb112765..0ac59d72ea8c 100644 --- a/hbase-shell/src/test/ruby/hbase/admin_test.rb +++ b/hbase-shell/src/test/ruby/hbase/admin_test.rb @@ -477,6 +477,7 @@ def teardown define_test "create should be able to set table options" do drop_test_table(@create_test_name) command(:create, @create_test_name, 'a', 'b', 'MAX_FILESIZE' => 12345678, + ERASURE_CODING_POLICY => 'RS-6-3-1024K' PRIORITY => '77', FLUSH_POLICY => 'org.apache.hadoop.hbase.regionserver.FlushAllLargeStoresPolicy', REGION_MEMSTORE_REPLICATION => 'TRUE', @@ -964,6 +965,26 @@ def teardown assert_match(/12345678/, admin.describe(@test_name)) end + define_test "alter should be able to change EC policy" do + command(:alter, @test_name, METHOD => 'table_att', 'ERASURE_CODING_POLICY' => "RS-6-3-1024") + assert_match(/RS-6-3-1024/, admin.describe(@test_name)) + end + + define_test "alter should be able to remove EC policy" do + command(:alter, @test_name, METHOD => 'table_att_unset', NAME => 'ERASURE_CODING_POLICY') + assert_not_match(/ERASURE_CODING_POLICY/, admin.describe(@test_name)) + end + + define_test "alter should be able to change EC POLICY w/o table_att" do + command(:alter, @test_name, 'ERASURE_CODING_POLICY' => "RS-6-3-1024") + assert_match(/RS-6-3-1024/, admin.describe(@test_name)) + end + + define_test "alter should be able to remove EC POLICY w/o table_att" do + command(:alter, @test_name, 'ERASURE_CODING_POLICY' => "null") + assert_not_match(/ERASURE_CODING_POLICY/, admin.describe(@test_name)) + end + define_test "alter should be able to specify coprocessor attributes with spec string" do drop_test_table(@test_name) create_test_table(@test_name) From e646e5da5cfb37ec0e124fab17f91adf09ed0334 Mon Sep 17 00:00:00 2001 From: Bryan Beaudreault Date: Mon, 18 Dec 2023 14:22:34 -0500 Subject: [PATCH 5/7] Add support in snapshot restore, fix shell support, fix tests, refactor per PR feedback --- .../server/master/MasterProcedure.proto | 4 +- .../hadoop/hbase/fs/ErasureCodingUtils.java | 96 ++++++++++-- .../procedure/CloneSnapshotProcedure.java | 11 ++ .../procedure/CreateTableProcedure.java | 20 +-- .../procedure/ModifyTableProcedure.java | 55 +++---- .../procedure/RestoreSnapshotProcedure.java | 33 +++- .../master/snapshot/SnapshotManager.java | 3 +- .../CompactedHFilesDischarger.java | 3 + .../hbase/util/TableDescriptorChecker.java | 79 +++++----- .../TestManageTableErasureCodingPolicy.java | 144 +++++++++++++----- .../hbase/client/AbstractTestShell.java | 8 +- hbase-shell/src/test/ruby/hbase/admin_test.rb | 17 ++- 12 files changed, 321 insertions(+), 152 deletions(-) diff --git a/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto b/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto index 24919977da6e..42eab7a642f0 100644 --- a/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto +++ b/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto @@ -56,7 +56,7 @@ enum CreateTableState { CREATE_TABLE_ASSIGN_REGIONS = 4; CREATE_TABLE_UPDATE_DESC_CACHE = 5; CREATE_TABLE_POST_OPERATION = 6; - CREATE_TABLE_SYNC_ERASURE_CODING_POLICY = 7; + CREATE_TABLE_SET_ERASURE_CODING_POLICY = 7; } message CreateTableStateData { @@ -269,6 +269,7 @@ enum CloneSnapshotState { CLONE_SNAPSHOT_UPDATE_DESC_CACHE = 5; CLONE_SNAPSHOT_POST_OPERATION = 6; CLONE_SNAPHOST_RESTORE_ACL = 7; + CLONE_SNAPSHOT_SET_ERASURE_CODING_POLICY = 8; } message CloneSnapshotStateData { @@ -287,6 +288,7 @@ enum RestoreSnapshotState { RESTORE_SNAPSHOT_WRITE_FS_LAYOUT = 3; RESTORE_SNAPSHOT_UPDATE_META = 4; RESTORE_SNAPSHOT_RESTORE_ACL = 5; + RESTORE_SNAPSHOT_SYNC_ERASURE_CODING_POLICY = 6; } message RestoreSnapshotStateData { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/fs/ErasureCodingUtils.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/fs/ErasureCodingUtils.java index 7c337afda74c..2983ecd6b6ba 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/fs/ErasureCodingUtils.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/fs/ErasureCodingUtils.java @@ -19,10 +19,18 @@ import java.io.IOException; import java.util.Collection; +import java.util.Objects; import java.util.stream.Collectors; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FSDataOutputStream; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.DoNotRetryIOException; import org.apache.hadoop.hbase.HBaseIOException; +import org.apache.hadoop.hbase.HConstants; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.TableDescriptor; +import org.apache.hadoop.hbase.util.CommonFSUtils; import org.apache.hadoop.hdfs.DistributedFileSystem; import org.apache.hadoop.hdfs.protocol.ErasureCodingPolicyInfo; import org.apache.yetus.audience.InterfaceAudience; @@ -39,33 +47,88 @@ private ErasureCodingUtils() { /** * Runs checks against the FileSystem, verifying that HDFS is supported and the policy is - * available and enabled. + * available, enabled, and works with a simple write. */ - public static void checkAvailable(FileSystem fs, String policy) throws HBaseIOException { - DistributedFileSystem dfs = getDfs(fs); + public static void verifySupport(Configuration conf, String policy) throws HBaseIOException { + DistributedFileSystem dfs = getDfs(conf); + checkAvailable(dfs, policy); + + Path globalTempDir = new Path(conf.get(HConstants.HBASE_DIR), HConstants.HBASE_TEMP_DIRECTORY); + Path currentTempDir = createTempDir(dfs, globalTempDir); + try { + setPolicy(dfs, currentTempDir, policy); + try (FSDataOutputStream out = dfs.create(new Path(currentTempDir, "test.out"))) { + out.writeUTF("Testing " + policy); + } + } catch (IOException e) { + throw new DoNotRetryIOException("Failed write test for EC policy. Check cause or logs", e); + } finally { + try { + dfs.delete(currentTempDir, true); + } catch (IOException e) { + LOG.warn("Failed to delete temp path for ec test", e); + } + } + } + private static Path createTempDir(FileSystem fs, Path tempDir) throws HBaseIOException { + Path currentTempDir = new Path(tempDir, "ec-test-" + System.currentTimeMillis()); + try { + fs.mkdirs(currentTempDir); + fs.deleteOnExit(currentTempDir); + } catch (IOException e) { + throw new HBaseIOException("Failed to create test dir for EC write test", e); + } + return currentTempDir; + } + + private static void checkAvailable(DistributedFileSystem dfs, String policy) + throws HBaseIOException { Collection policies; try { policies = dfs.getAllErasureCodingPolicies(); } catch (IOException e) { throw new HBaseIOException("Failed to check for Erasure Coding policy: " + policy, e); } - for (ErasureCodingPolicyInfo policyInfo : policies) { if (policyInfo.getPolicy().getName().equals(policy)) { if (!policyInfo.isEnabled()) { - throw new HBaseIOException("Cannot set Erasure Coding policy: " + policy + throw new DoNotRetryIOException("Cannot set Erasure Coding policy: " + policy + ". The policy must be enabled, but has state " + policyInfo.getState()); } return; } } - - throw new HBaseIOException( + throw new DoNotRetryIOException( "Cannot set Erasure Coding policy: " + policy + ". Policy not found. Available policies are: " + policies.stream().map(p -> p.getPolicy().getName()).collect(Collectors.joining(", "))); } + public static boolean needsSync(TableDescriptor oldDescriptor, TableDescriptor newDescriptor) { + String newPolicy = oldDescriptor.getErasureCodingPolicy(); + String oldPolicy = newDescriptor.getErasureCodingPolicy(); + return !Objects.equals(oldPolicy, newPolicy); + } + + public static void sync(FileSystem fs, Path rootDir, TableDescriptor newDescriptor) + throws IOException { + String newPolicy = newDescriptor.getErasureCodingPolicy(); + if (newPolicy == null) { + unsetPolicy(fs, rootDir, newDescriptor.getTableName()); + } else { + setPolicy(fs, rootDir, newDescriptor.getTableName(), newPolicy); + } + } + + /** + * Sets the EC policy on the table directory for the specified table + */ + public static void setPolicy(FileSystem fs, Path rootDir, TableName tableName, String policy) + throws IOException { + Path path = CommonFSUtils.getTableDir(rootDir, tableName); + setPolicy(fs, path, policy); + } + /** * Sets the EC policy on the path */ @@ -76,8 +139,10 @@ public static void setPolicy(FileSystem fs, Path path, String policy) throws IOE /** * Unsets any EC policy specified on the path. */ - public static void unsetPolicy(FileSystem fs, Path path) throws IOException { + public static void unsetPolicy(FileSystem fs, Path rootDir, TableName tableName) + throws IOException { DistributedFileSystem dfs = getDfs(fs); + Path path = CommonFSUtils.getTableDir(rootDir, tableName); if (dfs.getErasureCodingPolicy(path) == null) { LOG.warn("No EC policy set for path {}, nothing to unset", path); return; @@ -85,9 +150,20 @@ public static void unsetPolicy(FileSystem fs, Path path) throws IOException { dfs.unsetErasureCodingPolicy(path); } - private static DistributedFileSystem getDfs(FileSystem fs) throws HBaseIOException { + private static DistributedFileSystem getDfs(Configuration conf) throws HBaseIOException { + try { + return getDfs(FileSystem.get(conf)); + } catch (DoNotRetryIOException e) { + throw e; + } catch (IOException e) { + throw new HBaseIOException("Failed to get FileSystem from conf", e); + } + + } + + private static DistributedFileSystem getDfs(FileSystem fs) throws DoNotRetryIOException { if (!(fs instanceof DistributedFileSystem)) { - throw new HBaseIOException( + throw new DoNotRetryIOException( "Cannot manage Erasure Coding policy. Erasure Coding is only available on HDFS, but fs is " + fs.getClass().getSimpleName()); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java index 1a9c93b82c03..a2c4cd40c9c1 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java @@ -36,6 +36,7 @@ import org.apache.hadoop.hbase.client.TableDescriptorBuilder; import org.apache.hadoop.hbase.errorhandling.ForeignException; import org.apache.hadoop.hbase.errorhandling.ForeignExceptionDispatcher; +import org.apache.hadoop.hbase.fs.ErasureCodingUtils; import org.apache.hadoop.hbase.master.MasterCoprocessorHost; import org.apache.hadoop.hbase.master.MasterFileSystem; import org.apache.hadoop.hbase.master.MetricsSnapshot; @@ -155,6 +156,16 @@ protected Flow executeFromState(final MasterProcedureEnv env, final CloneSnapsho updateTableDescriptorWithSFT(); newRegions = createFilesystemLayout(env, tableDescriptor, newRegions); env.getMasterServices().getTableDescriptors().update(tableDescriptor, true); + if (tableDescriptor.getErasureCodingPolicy() != null) { + setNextState(CloneSnapshotState.CLONE_SNAPSHOT_SET_ERASURE_CODING_POLICY); + } else { + setNextState(CloneSnapshotState.CLONE_SNAPSHOT_ADD_TO_META); + } + break; + case CLONE_SNAPSHOT_SET_ERASURE_CODING_POLICY: + ErasureCodingUtils.setPolicy(env.getMasterFileSystem().getFileSystem(), + env.getMasterFileSystem().getRootDir(), getTableName(), + tableDescriptor.getErasureCodingPolicy()); setNextState(CloneSnapshotState.CLONE_SNAPSHOT_ADD_TO_META); break; case CLONE_SNAPSHOT_ADD_TO_META: 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 53896c31bd50..533b6fffcc43 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 @@ -99,15 +99,16 @@ protected Flow executeFromState(final MasterProcedureEnv env, final CreateTableS DeleteTableProcedure.deleteFromFs(env, getTableName(), newRegions, true); newRegions = createFsLayout(env, tableDescriptor, newRegions); env.getMasterServices().getTableDescriptors().update(tableDescriptor, true); - setNextState(CreateTableState.CREATE_TABLE_SYNC_ERASURE_CODING_POLICY); - break; - case CREATE_TABLE_SYNC_ERASURE_CODING_POLICY: if (tableDescriptor.getErasureCodingPolicy() != null) { - final Path tableDir = CommonFSUtils.getTableDir(env.getMasterFileSystem().getRootDir(), - tableDescriptor.getTableName()); - ErasureCodingUtils.setPolicy(env.getMasterFileSystem().getFileSystem(), tableDir, - tableDescriptor.getErasureCodingPolicy()); + setNextState(CreateTableState.CREATE_TABLE_SET_ERASURE_CODING_POLICY); + } else { + setNextState(CreateTableState.CREATE_TABLE_ADD_TO_META); } + break; + case CREATE_TABLE_SET_ERASURE_CODING_POLICY: + ErasureCodingUtils.setPolicy(env.getMasterFileSystem().getFileSystem(), + env.getMasterFileSystem().getRootDir(), getTableName(), + tableDescriptor.getErasureCodingPolicy()); setNextState(CreateTableState.CREATE_TABLE_ADD_TO_META); break; case CREATE_TABLE_ADD_TO_META: @@ -283,11 +284,6 @@ private boolean prepareCreate(final MasterProcedureEnv env) throws IOException { StoreFileTrackerValidationUtils.checkForCreateTable(env.getMasterConfiguration(), tableDescriptor); - if (tableDescriptor.getErasureCodingPolicy() != null) { - ErasureCodingUtils.checkAvailable(env.getMasterFileSystem().getFileSystem(), - tableDescriptor.getErasureCodingPolicy()); - } - return true; } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java index a79a6389b9fb..9a52dbd079dd 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java @@ -27,14 +27,11 @@ import java.util.Collections; import java.util.HashSet; import java.util.List; -import java.util.Objects; import java.util.Set; import java.util.function.Supplier; import java.util.stream.Collectors; import java.util.stream.IntStream; import org.apache.hadoop.conf.Configuration; -import org.apache.hadoop.fs.FileSystem; -import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.ConcurrentTableModificationException; import org.apache.hadoop.hbase.DoNotRetryIOException; import org.apache.hadoop.hbase.HBaseIOException; @@ -52,7 +49,6 @@ import org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerValidationUtils; import org.apache.hadoop.hbase.rsgroup.RSGroupInfo; import org.apache.hadoop.hbase.util.Bytes; -import org.apache.hadoop.hbase.util.CommonFSUtils; import org.apache.yetus.audience.InterfaceAudience; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -120,11 +116,6 @@ protected void preflightChecks(MasterProcedureEnv env, Boolean enabled) throws H } } - String policy = modifiedTableDescriptor.getErasureCodingPolicy(); - if (policy != null) { - ErasureCodingUtils.checkAvailable(env.getMasterFileSystem().getFileSystem(), policy); - } - if (!reopenRegions) { if (this.unmodifiedTableDescriptor == null) { throw new HBaseIOException( @@ -230,9 +221,12 @@ protected Flow executeFromState(final MasterProcedureEnv env, final ModifyTableS postModify(env, state); if (reopenRegions) { setNextState(ModifyTableState.MODIFY_TABLE_REOPEN_ALL_REGIONS); - } else { - setNextState(ModifyTableState.MODIFY_TABLE_SYNC_ERASURE_CODING_POLICY); - } + } else + if (ErasureCodingUtils.needsSync(unmodifiedTableDescriptor, modifiedTableDescriptor)) { + setNextState(ModifyTableState.MODIFY_TABLE_SYNC_ERASURE_CODING_POLICY); + } else { + return Flow.NO_MORE_STATE; + } break; case MODIFY_TABLE_REOPEN_ALL_REGIONS: if (isTableEnabled(env)) { @@ -256,16 +250,24 @@ protected Flow executeFromState(final MasterProcedureEnv env, final ModifyTableS } if (deleteColumnFamilyInModify) { setNextState(ModifyTableState.MODIFY_TABLE_DELETE_FS_LAYOUT); - } else { - setNextState(ModifyTableState.MODIFY_TABLE_SYNC_ERASURE_CODING_POLICY); - } + } else + if (ErasureCodingUtils.needsSync(unmodifiedTableDescriptor, modifiedTableDescriptor)) { + setNextState(ModifyTableState.MODIFY_TABLE_SYNC_ERASURE_CODING_POLICY); + } else { + return Flow.NO_MORE_STATE; + } break; case MODIFY_TABLE_DELETE_FS_LAYOUT: deleteFromFs(env, unmodifiedTableDescriptor, modifiedTableDescriptor); - setNextState(ModifyTableState.MODIFY_TABLE_SYNC_ERASURE_CODING_POLICY); - break; + if (ErasureCodingUtils.needsSync(unmodifiedTableDescriptor, modifiedTableDescriptor)) { + setNextState(ModifyTableState.MODIFY_TABLE_SYNC_ERASURE_CODING_POLICY); + break; + } else { + return Flow.NO_MORE_STATE; + } case MODIFY_TABLE_SYNC_ERASURE_CODING_POLICY: - syncErasureCodingPolicy(env); + ErasureCodingUtils.sync(env.getMasterFileSystem().getFileSystem(), + env.getMasterFileSystem().getRootDir(), modifiedTableDescriptor); return Flow.NO_MORE_STATE; default: throw new UnsupportedOperationException("unhandled state=" + state); @@ -527,23 +529,6 @@ private void closeExcessReplicasIfNeeded(MasterProcedureEnv env) { .createUnassignProceduresForClosingExcessRegionReplicas(getTableName(), newReplicaCount)); } - private void syncErasureCodingPolicy(MasterProcedureEnv env) throws IOException { - String oldPolicy = unmodifiedTableDescriptor.getErasureCodingPolicy(); - String newPolicy = modifiedTableDescriptor.getErasureCodingPolicy(); - if (Objects.equals(oldPolicy, newPolicy)) { - return; - } - - FileSystem fileSystem = env.getMasterFileSystem().getFileSystem(); - Path tableDir = CommonFSUtils.getTableDir(env.getMasterFileSystem().getRootDir(), - unmodifiedTableDescriptor.getTableName()); - if (newPolicy == null) { - ErasureCodingUtils.unsetPolicy(fileSystem, tableDir); - } else { - ErasureCodingUtils.setPolicy(fileSystem, tableDir, newPolicy); - } - } - /** * Action after modifying table. * @param env MasterProcedureEnv diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java index bc2f41c13910..3c6a59dfda5e 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java @@ -39,6 +39,7 @@ import org.apache.hadoop.hbase.errorhandling.ForeignException; import org.apache.hadoop.hbase.errorhandling.ForeignExceptionDispatcher; import org.apache.hadoop.hbase.favored.FavoredNodesManager; +import org.apache.hadoop.hbase.fs.ErasureCodingUtils; import org.apache.hadoop.hbase.master.MasterFileSystem; import org.apache.hadoop.hbase.master.MetricsSnapshot; import org.apache.hadoop.hbase.master.RegionState; @@ -70,6 +71,7 @@ public class RestoreSnapshotProcedure extends AbstractStateMachineTableProcedure { private static final Logger LOG = LoggerFactory.getLogger(RestoreSnapshotProcedure.class); + private TableDescriptor oldTableDescriptor; private TableDescriptor modifiedTableDescriptor; private List regionsToRestore = null; private List regionsToRemove = null; @@ -94,18 +96,25 @@ public RestoreSnapshotProcedure(final MasterProcedureEnv env, this(env, tableDescriptor, snapshot, false); } + public RestoreSnapshotProcedure(final MasterProcedureEnv env, + final TableDescriptor tableDescriptor, final SnapshotDescription snapshot, + final boolean restoreAcl) throws HBaseIOException { + this(env, tableDescriptor, tableDescriptor, snapshot, restoreAcl); + } + /** * Constructor - * @param env MasterProcedureEnv - * @param tableDescriptor the table to operate on - * @param snapshot snapshot to restore from + * @param env MasterProcedureEnv + * @param modifiedTableDescriptor the table to operate on + * @param snapshot snapshot to restore from */ public RestoreSnapshotProcedure(final MasterProcedureEnv env, - final TableDescriptor tableDescriptor, final SnapshotDescription snapshot, - final boolean restoreAcl) throws HBaseIOException { + final TableDescriptor oldTableDescriptor, final TableDescriptor modifiedTableDescriptor, + final SnapshotDescription snapshot, final boolean restoreAcl) throws HBaseIOException { super(env); + this.oldTableDescriptor = oldTableDescriptor; // This is the new schema we are going to write out as this modification. - this.modifiedTableDescriptor = tableDescriptor; + this.modifiedTableDescriptor = modifiedTableDescriptor; preflightChecks(env, null/* Table can be online when restore is called? */); // Snapshot information this.snapshot = snapshot; @@ -143,8 +152,18 @@ protected Flow executeFromState(final MasterProcedureEnv env, final RestoreSnaps break; case RESTORE_SNAPSHOT_UPDATE_TABLE_DESCRIPTOR: updateTableDescriptor(env); - setNextState(RestoreSnapshotState.RESTORE_SNAPSHOT_WRITE_FS_LAYOUT); + // for restore, table dir already exists. sync EC if necessary before doing the real + // restore. this may be useful in certain restore scenarios where a user is explicitly + // trying to disable EC for some reason as part of the restore. + if (ErasureCodingUtils.needsSync(oldTableDescriptor, modifiedTableDescriptor)) { + setNextState(RestoreSnapshotState.RESTORE_SNAPSHOT_SYNC_ERASURE_CODING_POLICY); + } else { + setNextState(RestoreSnapshotState.RESTORE_SNAPSHOT_WRITE_FS_LAYOUT); + } break; + case RESTORE_SNAPSHOT_SYNC_ERASURE_CODING_POLICY: + ErasureCodingUtils.sync(env.getMasterFileSystem().getFileSystem(), + env.getMasterFileSystem().getRootDir(), modifiedTableDescriptor); case RESTORE_SNAPSHOT_WRITE_FS_LAYOUT: restoreSnapshot(env); setNextState(RestoreSnapshotState.RESTORE_SNAPSHOT_UPDATE_META); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java index ed7ef583ec52..14c7b4925de2 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java @@ -1086,9 +1086,10 @@ private synchronized long restoreSnapshot(final SnapshotDescription snapshot, } try { + TableDescriptor oldDescriptor = master.getTableDescriptors().get(tableName); long procId = master.getMasterProcedureExecutor().submitProcedure( new RestoreSnapshotProcedure(master.getMasterProcedureExecutor().getEnvironment(), - tableDescriptor, snapshot, restoreAcl), + oldDescriptor, tableDescriptor, snapshot, restoreAcl), nonceKey); this.restoreTableToProcIdMap.put(tableName, procId); return procId; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactedHFilesDischarger.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactedHFilesDischarger.java index 2f4eaa085378..20c0d93b4708 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactedHFilesDischarger.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactedHFilesDischarger.java @@ -17,6 +17,7 @@ */ package org.apache.hadoop.hbase.regionserver; +import com.google.errorprone.annotations.RestrictedApi; import java.util.List; import org.apache.hadoop.hbase.ScheduledChore; import org.apache.hadoop.hbase.Server; @@ -70,6 +71,8 @@ public CompactedHFilesDischarger(final int period, final Stoppable stopper, * no-executor before you call run. * @return The old setting for useExecutor */ + @RestrictedApi(explanation = "Should only be called in tests", link = "", + allowedOnPath = ".*/src/test/.*") public boolean setUseExecutor(final boolean useExecutor) { boolean oldSetting = this.useExecutor; this.useExecutor = useExecutor; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/TableDescriptorChecker.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/TableDescriptorChecker.java index e7080fc9323f..f7f61dd528e5 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/TableDescriptorChecker.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/TableDescriptorChecker.java @@ -27,6 +27,7 @@ import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder; import org.apache.hadoop.hbase.client.TableDescriptor; import org.apache.hadoop.hbase.client.TableDescriptorBuilder; +import org.apache.hadoop.hbase.fs.ErasureCodingUtils; import org.apache.hadoop.hbase.regionserver.DefaultStoreEngine; import org.apache.hadoop.hbase.regionserver.HStore; import org.apache.hadoop.hbase.regionserver.RegionCoprocessorHost; @@ -145,6 +146,11 @@ public static void sanityCheck(final Configuration c, final TableDescriptor td) // check bloom filter type checkBloomFilterType(conf, td); + if (td.getErasureCodingPolicy() != null) { + warnOrThrowExceptionForFailure(logWarn, + () -> ErasureCodingUtils.verifySupport(conf, td.getErasureCodingPolicy())); + } + for (ColumnFamilyDescriptor hcd : td.getColumnFamilies()) { if (hcd.getTimeToLive() <= 0) { String message = "TTL for column family " + hcd.getNameAsString() + " must be positive."; @@ -184,19 +190,13 @@ public static void sanityCheck(final Configuration c, final TableDescriptor td) } // check in-memory compaction - try { - hcd.getInMemoryCompaction(); - } catch (IllegalArgumentException e) { - warnOrThrowExceptionForFailure(logWarn, e.getMessage(), e); - } + warnOrThrowExceptionForFailure(logWarn, hcd::getInMemoryCompaction); } } private static void checkReplicationScope(final Configuration conf, final TableDescriptor td) throws IOException { - // Setting logs to warning instead of throwing exception if sanityChecks are disabled - boolean logWarn = !shouldSanityCheck(conf); - try { + warnOrThrowExceptionForFailure(conf, () -> { for (ColumnFamilyDescriptor cfd : td.getColumnFamilies()) { // check replication scope WALProtos.ScopeType scop = WALProtos.ScopeType.valueOf(cfd.getScope()); @@ -207,15 +207,12 @@ private static void checkReplicationScope(final Configuration conf, final TableD throw new DoNotRetryIOException(message); } } - } catch (IOException e) { - warnOrThrowExceptionForFailure(logWarn, e.getMessage(), e); - } - + }); } private static void checkCompactionPolicy(final Configuration conf, final TableDescriptor td) throws IOException { - try { + warnOrThrowExceptionForFailure(conf, () -> { // FIFO compaction has some requirements // Actually FCP ignores periodic major compactions String className = td.getValue(DefaultStoreEngine.DEFAULT_COMPACTION_POLICY_CLASS_KEY); @@ -268,16 +265,12 @@ private static void checkCompactionPolicy(final Configuration conf, final TableD throw new IOException(message); } } - } catch (IOException e) { - warnOrThrowExceptionForFailure(false, e.getMessage(), e); - } + }); } private static void checkBloomFilterType(final Configuration conf, final TableDescriptor td) throws IOException { - // Setting logs to warning instead of throwing exception if sanityChecks are disabled - boolean logWarn = !shouldSanityCheck(conf); - try { + warnOrThrowExceptionForFailure(conf, () -> { for (ColumnFamilyDescriptor cfd : td.getColumnFamilies()) { Configuration cfdConf = new CompoundConfiguration().addStringMap(cfd.getConfiguration()); try { @@ -286,50 +279,36 @@ private static void checkBloomFilterType(final Configuration conf, final TableDe throw new DoNotRetryIOException("Failed to get bloom filter param", e); } } - } catch (IOException e) { - warnOrThrowExceptionForFailure(logWarn, e.getMessage(), e); - } + }); } public static void checkCompression(final Configuration conf, final TableDescriptor td) throws IOException { - // Setting logs to warning instead of throwing exception if sanityChecks are disabled - boolean logWarn = !shouldSanityCheck(conf); - try { + warnOrThrowExceptionForFailure(conf, () -> { for (ColumnFamilyDescriptor cfd : td.getColumnFamilies()) { CompressionTest.testCompression(cfd.getCompressionType()); CompressionTest.testCompression(cfd.getCompactionCompressionType()); CompressionTest.testCompression(cfd.getMajorCompactionCompressionType()); CompressionTest.testCompression(cfd.getMinorCompactionCompressionType()); } - } catch (IOException e) { - warnOrThrowExceptionForFailure(logWarn, e.getMessage(), e); - } + }); } public static void checkEncryption(final Configuration conf, final TableDescriptor td) throws IOException { - // Setting logs to warning instead of throwing exception if sanityChecks are disabled - boolean logWarn = !shouldSanityCheck(conf); - try { + warnOrThrowExceptionForFailure(conf, () -> { for (ColumnFamilyDescriptor cfd : td.getColumnFamilies()) { EncryptionTest.testEncryption(conf, cfd.getEncryptionType(), cfd.getEncryptionKey()); } - } catch (IOException e) { - warnOrThrowExceptionForFailure(logWarn, e.getMessage(), e); - } + }); } public static void checkClassLoading(final Configuration conf, final TableDescriptor td) throws IOException { - // Setting logs to warning instead of throwing exception if sanityChecks are disabled - boolean logWarn = !shouldSanityCheck(conf); - try { + warnOrThrowExceptionForFailure(conf, () -> { RegionSplitPolicy.getSplitPolicyClass(td, conf); RegionCoprocessorHost.testTableCoprocessorAttrs(conf, td); - } catch (Exception e) { - warnOrThrowExceptionForFailure(logWarn, e.getMessage(), e); - } + }); } // HBASE-13350 - Helper method to log warning on sanity check failures if checks disabled. @@ -341,4 +320,24 @@ private static void warnOrThrowExceptionForFailure(boolean logWarn, String messa } LOG.warn(message); } + + private static void warnOrThrowExceptionForFailure(Configuration conf, ThrowingRunnable runnable) + throws IOException { + boolean logWarn = !shouldSanityCheck(conf); + warnOrThrowExceptionForFailure(logWarn, runnable); + } + + private static void warnOrThrowExceptionForFailure(boolean logWarn, ThrowingRunnable runnable) + throws IOException { + try { + runnable.run(); + } catch (Exception e) { + warnOrThrowExceptionForFailure(logWarn, e.getMessage(), e); + } + } + + @FunctionalInterface + interface ThrowingRunnable { + void run() throws Exception; + } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestManageTableErasureCodingPolicy.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestManageTableErasureCodingPolicy.java index 1fae7948ac13..91880c453a3f 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestManageTableErasureCodingPolicy.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestManageTableErasureCodingPolicy.java @@ -26,6 +26,7 @@ import static org.junit.Assert.assertThrows; import java.io.IOException; +import java.util.function.Function; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.LocatedFileStatus; import org.apache.hadoop.fs.Path; @@ -47,6 +48,7 @@ import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.util.CommonFSUtils; import org.apache.hadoop.hbase.util.JVMClusterUtil; +import org.apache.hadoop.hbase.util.TableDescriptorChecker; import org.apache.hadoop.hdfs.DistributedFileSystem; import org.apache.hadoop.hdfs.protocol.ErasureCodingPolicy; import org.junit.AfterClass; @@ -54,6 +56,8 @@ import org.junit.ClassRule; import org.junit.Test; import org.junit.experimental.categories.Category; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; @Category({ MasterTests.class, MediumTests.class }) public class TestManageTableErasureCodingPolicy { @@ -61,6 +65,8 @@ public class TestManageTableErasureCodingPolicy { @ClassRule public static final HBaseClassTestRule CLASS_RULE = HBaseClassTestRule.forClass(TestManageTableErasureCodingPolicy.class); + private static final Logger LOG = + LoggerFactory.getLogger(TestManageTableErasureCodingPolicy.class); private static final HBaseTestingUtil UTIL = new HBaseTestingUtil(); private static final byte[] FAMILY = Bytes.toBytes("a"); @@ -69,13 +75,18 @@ public class TestManageTableErasureCodingPolicy { .newBuilder(NON_EC_TABLE).setColumnFamily(ColumnFamilyDescriptorBuilder.of(FAMILY)).build(); private static final TableName EC_TABLE = TableName.valueOf("bar"); private static final TableDescriptor EC_TABLE_DESC = - TableDescriptorBuilder.newBuilder(EC_TABLE).setErasureCodingPolicy("RS-6-3-1024k") + TableDescriptorBuilder.newBuilder(EC_TABLE).setErasureCodingPolicy("XOR-2-1-1024k") .setColumnFamily(ColumnFamilyDescriptorBuilder.of(FAMILY)).build(); @BeforeClass public static void beforeClass() throws Exception { - UTIL.startMiniDFSCluster(6); // 6 necessary for RS-6-3-1024k + // enable because we are testing the checks below + UTIL.getConfiguration().setBoolean(TableDescriptorChecker.TABLE_SANITY_CHECKS, true); + UTIL.startMiniDFSCluster(3); // 3 necessary for XOR-2-1-1024k UTIL.startMiniCluster(1); + DistributedFileSystem fs = (DistributedFileSystem) FileSystem.get(UTIL.getConfiguration()); + fs.enableErasureCodingPolicy("XOR-2-1-1024k"); + fs.enableErasureCodingPolicy("RS-6-3-1024k"); Table table = UTIL.createTable(NON_EC_TABLE_DESC, null); UTIL.loadTable(table, FAMILY); UTIL.flush(); @@ -89,31 +100,31 @@ public static void afterClass() throws Exception { @Test public void itValidatesPolicyNameForCreate() { - HBaseIOException thrown = assertThrows(HBaseIOException.class, () -> { - try (Admin admin = UTIL.getAdmin()) { - admin.createTable( - TableDescriptorBuilder.newBuilder(EC_TABLE_DESC).setErasureCodingPolicy("foo").build()); - } - }); - assertThat(thrown.getMessage(), - containsString("Cannot set Erasure Coding policy: foo. Policy not found")); - - thrown = assertThrows(HBaseIOException.class, () -> { - try (Admin admin = UTIL.getAdmin()) { - admin.createTable(TableDescriptorBuilder.newBuilder(EC_TABLE_DESC) - .setErasureCodingPolicy("RS-10-4-1024k").build()); - } - }); - assertThat(thrown.getMessage(), containsString( - "Cannot set Erasure Coding policy: RS-10-4-1024k. The policy must be enabled")); + runValidatePolicyNameTest(unused -> EC_TABLE_DESC, Admin::createTable); } @Test public void itValidatesPolicyNameForAlter() { + runValidatePolicyNameTest(admin -> { + try { + return admin.getDescriptor(NON_EC_TABLE); + } catch (IOException e) { + throw new RuntimeException(e); + } + }, Admin::modifyTable); + } + + @FunctionalInterface + interface ThrowingTableDescriptorConsumer { + void accept(Admin admin, TableDescriptor desc) throws IOException; + } + + private void runValidatePolicyNameTest(Function descriptorSupplier, + ThrowingTableDescriptorConsumer consumer) { HBaseIOException thrown = assertThrows(HBaseIOException.class, () -> { try (Admin admin = UTIL.getAdmin()) { - TableDescriptor desc = UTIL.getAdmin().getDescriptor(NON_EC_TABLE); - admin.modifyTable( + TableDescriptor desc = descriptorSupplier.apply(admin); + consumer.accept(admin, TableDescriptorBuilder.newBuilder(desc).setErasureCodingPolicy("foo").build()); } }); @@ -122,13 +133,23 @@ public void itValidatesPolicyNameForAlter() { thrown = assertThrows(HBaseIOException.class, () -> { try (Admin admin = UTIL.getAdmin()) { - TableDescriptor desc = UTIL.getAdmin().getDescriptor(NON_EC_TABLE); - admin.modifyTable( + TableDescriptor desc = descriptorSupplier.apply(admin); + consumer.accept(admin, TableDescriptorBuilder.newBuilder(desc).setErasureCodingPolicy("RS-10-4-1024k").build()); } }); assertThat(thrown.getMessage(), containsString( "Cannot set Erasure Coding policy: RS-10-4-1024k. The policy must be enabled")); + + // RS-6-3-1024k requires at least 6 datanodes, so should fail write test + thrown = assertThrows(HBaseIOException.class, () -> { + try (Admin admin = UTIL.getAdmin()) { + TableDescriptor desc = descriptorSupplier.apply(admin); + consumer.accept(admin, + TableDescriptorBuilder.newBuilder(desc).setErasureCodingPolicy("RS-6-3-1024k").build()); + } + }); + assertThat(thrown.getMessage(), containsString("Failed write test for EC policy")); } @Test @@ -139,7 +160,7 @@ public void testCreateTableErasureCodingSync() throws IOException { UTIL.flush(EC_TABLE); Path rootDir = CommonFSUtils.getRootDir(UTIL.getConfiguration()); DistributedFileSystem dfs = (DistributedFileSystem) FileSystem.get(UTIL.getConfiguration()); - checkRegionDirAndFilePolicies(dfs, rootDir, EC_TABLE, "RS-6-3-1024k", "RS-6-3-1024k"); + checkRegionDirAndFilePolicies(dfs, rootDir, EC_TABLE, "XOR-2-1-1024k", "XOR-2-1-1024k"); } } @@ -155,31 +176,23 @@ public void testModifyTableErasureCodingSync() throws IOException, InterruptedEx // add EC TableDescriptor desc = UTIL.getAdmin().getDescriptor(NON_EC_TABLE); TableDescriptor newDesc = - TableDescriptorBuilder.newBuilder(desc).setErasureCodingPolicy("RS-6-3-1024k").build(); + TableDescriptorBuilder.newBuilder(desc).setErasureCodingPolicy("XOR-2-1-1024k").build(); admin.modifyTable(newDesc); // check dirs, but files should not be changed yet - checkRegionDirAndFilePolicies(dfs, rootDir, NON_EC_TABLE, "RS-6-3-1024k", null); + checkRegionDirAndFilePolicies(dfs, rootDir, NON_EC_TABLE, "XOR-2-1-1024k", null); - // compact to rewrite files with EC, then run discharger to get rid of the old non-EC files - UTIL.compact(NON_EC_TABLE, true); - for (JVMClusterUtil.RegionServerThread regionserver : UTIL.getHBaseCluster() - .getLiveRegionServerThreads()) { - CompactedHFilesDischarger chore = - regionserver.getRegionServer().getCompactedHFilesDischarger(); - chore.setUseExecutor(false); - chore.chore(); - } + compactAwayOldFiles(NON_EC_TABLE); // expect both dirs and files to be EC now - checkRegionDirAndFilePolicies(dfs, rootDir, NON_EC_TABLE, "RS-6-3-1024k", "RS-6-3-1024k"); + checkRegionDirAndFilePolicies(dfs, rootDir, NON_EC_TABLE, "XOR-2-1-1024k", "XOR-2-1-1024k"); newDesc = TableDescriptorBuilder.newBuilder(newDesc).setErasureCodingPolicy(null).build(); // remove EC now admin.modifyTable(newDesc); // dirs should no longer be EC, but old EC files remain - checkRegionDirAndFilePolicies(dfs, rootDir, NON_EC_TABLE, null, "RS-6-3-1024k"); + checkRegionDirAndFilePolicies(dfs, rootDir, NON_EC_TABLE, null, "XOR-2-1-1024k"); // compact to rewrite EC files without EC, then run discharger to get rid of the old EC files UTIL.compact(NON_EC_TABLE, true); @@ -195,6 +208,60 @@ public void testModifyTableErasureCodingSync() throws IOException, InterruptedEx } } + private void compactAwayOldFiles(TableName tableName) throws IOException { + LOG.info("Compacting and discharging files for {}", tableName); + // compact to rewrit files, then run discharger to get rid of the old files + UTIL.compact(tableName, true); + for (JVMClusterUtil.RegionServerThread regionserver : UTIL.getHBaseCluster() + .getLiveRegionServerThreads()) { + CompactedHFilesDischarger chore = + regionserver.getRegionServer().getCompactedHFilesDischarger(); + chore.setUseExecutor(false); + chore.chore(); + } + } + + @Test + public void testRestoreSnapshot() throws IOException { + String snapshotName = "testRestoreSnapshot_snap"; + TableName tableName = TableName.valueOf("testRestoreSnapshot_tbl"); + try (Admin admin = UTIL.getAdmin()) { + Path rootDir = CommonFSUtils.getRootDir(UTIL.getConfiguration()); + DistributedFileSystem dfs = (DistributedFileSystem) FileSystem.get(UTIL.getConfiguration()); + + // recreate EC test table and load it + if (admin.tableExists(EC_TABLE)) { + admin.disableTable(EC_TABLE); + admin.deleteTable(EC_TABLE); + } + admin.createTable(EC_TABLE_DESC); + try (Table table = UTIL.getConnection().getTable(EC_TABLE)) { + UTIL.loadTable(table, FAMILY); + } + + // Take a snapshot, then clone it into a new table + admin.snapshot(snapshotName, EC_TABLE); + admin.cloneSnapshot(snapshotName, tableName); + compactAwayOldFiles(tableName); + + // Verify the new table has the right EC policy + checkRegionDirAndFilePolicies(dfs, rootDir, tableName, "XOR-2-1-1024k", "XOR-2-1-1024k"); + + // Remove the EC policy from the EC test table, and verify that worked + admin.modifyTable( + TableDescriptorBuilder.newBuilder(EC_TABLE_DESC).setErasureCodingPolicy(null).build()); + compactAwayOldFiles(EC_TABLE); + checkRegionDirAndFilePolicies(dfs, rootDir, EC_TABLE, null, null); + + // Restore snapshot, and then verify it has the policy again + admin.disableTable(EC_TABLE); + admin.restoreSnapshot(snapshotName); + admin.enableTable(EC_TABLE); + compactAwayOldFiles(EC_TABLE); + checkRegionDirAndFilePolicies(dfs, rootDir, EC_TABLE, "XOR-2-1-1024k", "XOR-2-1-1024k"); + } + } + private void checkRegionDirAndFilePolicies(DistributedFileSystem dfs, Path rootDir, TableName testTable, String expectedDirPolicy, String expectedFilePolicy) throws IOException { Path tableDir = CommonFSUtils.getTableDir(rootDir, testTable); @@ -209,6 +276,7 @@ private void checkRegionDirAndFilePolicies(DistributedFileSystem dfs, Path rootD LocatedFileStatus fileStatus = itr.next(); Path path = fileStatus.getPath(); if (!HFile.isHFileFormat(dfs, path)) { + LOG.info("{} is not an hfile", path); continue; } filesMatched++; @@ -222,7 +290,7 @@ private void checkPolicy(DistributedFileSystem dfs, Path path, String expectedPo throws IOException { ErasureCodingPolicy policy = dfs.getErasureCodingPolicy(path); if (expectedPolicy == null) { - assertThat(policy, nullValue()); + assertThat("policy for " + path, policy, nullValue()); } else { assertThat("policy for " + path, policy, notNullValue()); assertThat("policy for " + path, policy.getName(), equalTo(expectedPolicy)); diff --git a/hbase-shell/src/test/java/org/apache/hadoop/hbase/client/AbstractTestShell.java b/hbase-shell/src/test/java/org/apache/hadoop/hbase/client/AbstractTestShell.java index 58ce051930d8..9270715fd8da 100644 --- a/hbase-shell/src/test/java/org/apache/hadoop/hbase/client/AbstractTestShell.java +++ b/hbase-shell/src/test/java/org/apache/hadoop/hbase/client/AbstractTestShell.java @@ -21,11 +21,13 @@ import java.util.ArrayList; import java.util.List; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.hbase.HBaseTestingUtil; import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.coprocessor.CoprocessorHost; import org.apache.hadoop.hbase.security.access.SecureTestUtil; import org.apache.hadoop.hbase.security.visibility.VisibilityTestUtil; +import org.apache.hadoop.hdfs.DistributedFileSystem; import org.jruby.embed.PathType; import org.jruby.embed.ScriptingContainer; import org.junit.AfterClass; @@ -99,7 +101,11 @@ public static void setUpBeforeClass() throws Exception { setUpConfig(); // Start mini cluster - TEST_UTIL.startMiniCluster(1); + // 3 datanodes needed for erasure coding checks + TEST_UTIL.startMiniCluster(3); + DistributedFileSystem dfs = + (DistributedFileSystem) FileSystem.get(TEST_UTIL.getConfiguration()); + dfs.enableErasureCodingPolicy("XOR-2-1-1024k"); setUpJRubyRuntime(); } diff --git a/hbase-shell/src/test/ruby/hbase/admin_test.rb b/hbase-shell/src/test/ruby/hbase/admin_test.rb index 0ac59d72ea8c..2734dc136734 100644 --- a/hbase-shell/src/test/ruby/hbase/admin_test.rb +++ b/hbase-shell/src/test/ruby/hbase/admin_test.rb @@ -422,7 +422,7 @@ def teardown define_test 'clear slowlog responses should work' do output = capture_stdout { command(:clear_slowlog_responses, nil) } - assert(output.include?('Cleared Slowlog responses from 0/1 RegionServers')) + assert(output.include?('Cleared Slowlog responses from 0/3 RegionServers')) end #------------------------------------------------------------------------------- @@ -477,7 +477,7 @@ def teardown define_test "create should be able to set table options" do drop_test_table(@create_test_name) command(:create, @create_test_name, 'a', 'b', 'MAX_FILESIZE' => 12345678, - ERASURE_CODING_POLICY => 'RS-6-3-1024K' + ERASURE_CODING_POLICY => 'XOR-2-1-1024k', PRIORITY => '77', FLUSH_POLICY => 'org.apache.hadoop.hbase.regionserver.FlushAllLargeStoresPolicy', REGION_MEMSTORE_REPLICATION => 'TRUE', @@ -487,6 +487,7 @@ def teardown MERGE_ENABLED => 'false') assert_equal(['a:', 'b:'], table(@create_test_name).get_all_columns.sort) assert_match(/12345678/, admin.describe(@create_test_name)) + assert_match(/XOR-2-1-1024k/, admin.describe(@create_test_name)) assert_match(/77/, admin.describe(@create_test_name)) assert_match(/'COMPACTION_ENABLED' => 'false'/, admin.describe(@create_test_name)) assert_match(/'SPLIT_ENABLED' => 'false'/, admin.describe(@create_test_name)) @@ -966,22 +967,24 @@ def teardown end define_test "alter should be able to change EC policy" do - command(:alter, @test_name, METHOD => 'table_att', 'ERASURE_CODING_POLICY' => "RS-6-3-1024") - assert_match(/RS-6-3-1024/, admin.describe(@test_name)) + command(:alter, @test_name, METHOD => 'table_att', 'ERASURE_CODING_POLICY' => "XOR-2-1-1024k") + assert_match(/XOR-2-1-1024k/, admin.describe(@test_name)) end define_test "alter should be able to remove EC policy" do + command(:alter, @test_name, METHOD => 'table_att', 'ERASURE_CODING_POLICY' => "XOR-2-1-1024k") command(:alter, @test_name, METHOD => 'table_att_unset', NAME => 'ERASURE_CODING_POLICY') assert_not_match(/ERASURE_CODING_POLICY/, admin.describe(@test_name)) end define_test "alter should be able to change EC POLICY w/o table_att" do - command(:alter, @test_name, 'ERASURE_CODING_POLICY' => "RS-6-3-1024") - assert_match(/RS-6-3-1024/, admin.describe(@test_name)) + command(:alter, @test_name, 'ERASURE_CODING_POLICY' => "XOR-2-1-1024k") + assert_match(/XOR-2-1-1024k/, admin.describe(@test_name)) end define_test "alter should be able to remove EC POLICY w/o table_att" do - command(:alter, @test_name, 'ERASURE_CODING_POLICY' => "null") + command(:alter, @test_name, 'ERASURE_CODING_POLICY' => "XOR-2-1-1024k") + command(:alter, @test_name, 'ERASURE_CODING_POLICY' => nil) assert_not_match(/ERASURE_CODING_POLICY/, admin.describe(@test_name)) end From 23fbc49dc58197e09b97a063e1f653a31c002c2d Mon Sep 17 00:00:00 2001 From: Bryan Beaudreault Date: Mon, 18 Dec 2023 14:35:37 -0500 Subject: [PATCH 6/7] small cleanup --- .../apache/hadoop/hbase/fs/ErasureCodingUtils.java | 13 +++++++++++++ .../master/procedure/RestoreSnapshotProcedure.java | 2 ++ 2 files changed, 15 insertions(+) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/fs/ErasureCodingUtils.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/fs/ErasureCodingUtils.java index 2983ecd6b6ba..6e3c1e9a7887 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/fs/ErasureCodingUtils.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/fs/ErasureCodingUtils.java @@ -53,6 +53,9 @@ public static void verifySupport(Configuration conf, String policy) throws HBase DistributedFileSystem dfs = getDfs(conf); checkAvailable(dfs, policy); + // Enable the policy on a test directory. Try writing ot it to ensure that HDFS allows it + // This acts as a safeguard against topology issues (not enough nodes for policy, etc) and + // anything else. This is otherwise hard to validate more directly. Path globalTempDir = new Path(conf.get(HConstants.HBASE_DIR), HConstants.HBASE_TEMP_DIRECTORY); Path currentTempDir = createTempDir(dfs, globalTempDir); try { @@ -104,12 +107,22 @@ private static void checkAvailable(DistributedFileSystem dfs, String policy) + policies.stream().map(p -> p.getPolicy().getName()).collect(Collectors.joining(", "))); } + /** + * Check if EC policy is different between two descriptors + * @return true if a sync is necessary + */ public static boolean needsSync(TableDescriptor oldDescriptor, TableDescriptor newDescriptor) { String newPolicy = oldDescriptor.getErasureCodingPolicy(); String oldPolicy = newDescriptor.getErasureCodingPolicy(); return !Objects.equals(oldPolicy, newPolicy); } + /** + * Sync the EC policy state from the newDescriptor onto the FS for the table dir of the provided + * table descriptor. If the policy is null, we will remove erasure coding from the FS for the + * table dir. If it's non-null, we'll set it to that policy. + * @param newDescriptor descriptor containing the policy and table name + */ public static void sync(FileSystem fs, Path rootDir, TableDescriptor newDescriptor) throws IOException { String newPolicy = newDescriptor.getErasureCodingPolicy(); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java index 3c6a59dfda5e..b1e990c8c58b 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java @@ -164,6 +164,8 @@ protected Flow executeFromState(final MasterProcedureEnv env, final RestoreSnaps case RESTORE_SNAPSHOT_SYNC_ERASURE_CODING_POLICY: ErasureCodingUtils.sync(env.getMasterFileSystem().getFileSystem(), env.getMasterFileSystem().getRootDir(), modifiedTableDescriptor); + setNextState(RestoreSnapshotState.RESTORE_SNAPSHOT_WRITE_FS_LAYOUT); + break; case RESTORE_SNAPSHOT_WRITE_FS_LAYOUT: restoreSnapshot(env); setNextState(RestoreSnapshotState.RESTORE_SNAPSHOT_UPDATE_META); From 4ec98a91b80d1e9d7f4842ec5dff3abb03c6c59f Mon Sep 17 00:00:00 2001 From: Bryan Beaudreault Date: Tue, 19 Dec 2023 10:14:18 -0500 Subject: [PATCH 7/7] review fixes --- .../protobuf/server/master/MasterProcedure.proto | 1 + .../procedure/RestoreSnapshotProcedure.java | 4 +++- .../hbase/util/TableDescriptorChecker.java | 2 +- hbase-shell/src/main/ruby/hbase/admin.rb | 3 ++- hbase-shell/src/test/ruby/hbase/admin_test.rb | 16 ++++++++-------- 5 files changed, 15 insertions(+), 11 deletions(-) diff --git a/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto b/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto index 42eab7a642f0..c562a4e5c2fe 100644 --- a/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto +++ b/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto @@ -300,6 +300,7 @@ message RestoreSnapshotStateData { repeated RegionInfo region_info_for_add = 6; repeated RestoreParentToChildRegionsPair parent_to_child_regions_pair_list = 7; optional bool restore_acl = 8; + required TableSchema old_table_schema = 9; } enum DispatchMergingRegionsState { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java index b1e990c8c58b..e16b33741065 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java @@ -260,7 +260,8 @@ protected void serializeStateData(ProcedureStateSerializer serializer) throws IO RestoreSnapshotStateData.Builder restoreSnapshotMsg = RestoreSnapshotStateData.newBuilder() .setUserInfo(MasterProcedureUtil.toProtoUserInfo(getUser())).setSnapshot(this.snapshot) - .setModifiedTableSchema(ProtobufUtil.toTableSchema(modifiedTableDescriptor)); + .setModifiedTableSchema(ProtobufUtil.toTableSchema(modifiedTableDescriptor)) + .setOldTableSchema(ProtobufUtil.toTableSchema(oldTableDescriptor)); if (regionsToRestore != null) { for (RegionInfo hri : regionsToRestore) { @@ -302,6 +303,7 @@ protected void deserializeStateData(ProcedureStateSerializer serializer) throws serializer.deserialize(RestoreSnapshotStateData.class); setUser(MasterProcedureUtil.toUserInfo(restoreSnapshotMsg.getUserInfo())); snapshot = restoreSnapshotMsg.getSnapshot(); + oldTableDescriptor = ProtobufUtil.toTableDescriptor(restoreSnapshotMsg.getOldTableSchema()); modifiedTableDescriptor = ProtobufUtil.toTableDescriptor(restoreSnapshotMsg.getModifiedTableSchema()); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/TableDescriptorChecker.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/TableDescriptorChecker.java index f7f61dd528e5..94e2e4bbfa08 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/TableDescriptorChecker.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/TableDescriptorChecker.java @@ -212,7 +212,7 @@ private static void checkReplicationScope(final Configuration conf, final TableD private static void checkCompactionPolicy(final Configuration conf, final TableDescriptor td) throws IOException { - warnOrThrowExceptionForFailure(conf, () -> { + warnOrThrowExceptionForFailure(false, () -> { // FIFO compaction has some requirements // Actually FCP ignores periodic major compactions String className = td.getValue(DefaultStoreEngine.DEFAULT_COMPACTION_POLICY_CLASS_KEY); diff --git a/hbase-shell/src/main/ruby/hbase/admin.rb b/hbase-shell/src/main/ruby/hbase/admin.rb index cf9d5dc65ca6..b04a79229831 100644 --- a/hbase-shell/src/main/ruby/hbase/admin.rb +++ b/hbase-shell/src/main/ruby/hbase/admin.rb @@ -1580,7 +1580,8 @@ def list_locks # Parse arguments and update TableDescriptorBuilder accordingly # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity def update_tdb_from_arg(tdb, arg) - tdb.setErasureCodingPolicy(arg.delete(TableDescriptorBuilder::ERASURE_CODING_POLICY)) if arg.include?(TableDescriptorBuilder::ERASURE_CODING_POLICY) + tdb.setErasureCodingPolicy(arg.delete(TableDescriptorBuilder::ERASURE_CODING_POLICY)) \ + if arg.include?(TableDescriptorBuilder::ERASURE_CODING_POLICY) tdb.setMaxFileSize(arg.delete(TableDescriptorBuilder::MAX_FILESIZE)) if arg.include?(TableDescriptorBuilder::MAX_FILESIZE) tdb.setReadOnly(JBoolean.valueOf(arg.delete(TableDescriptorBuilder::READONLY))) if arg.include?(TableDescriptorBuilder::READONLY) tdb.setCompactionEnabled(JBoolean.valueOf(arg.delete(TableDescriptorBuilder::COMPACTION_ENABLED))) if arg.include?(TableDescriptorBuilder::COMPACTION_ENABLED) diff --git a/hbase-shell/src/test/ruby/hbase/admin_test.rb b/hbase-shell/src/test/ruby/hbase/admin_test.rb index 2734dc136734..14f393247217 100644 --- a/hbase-shell/src/test/ruby/hbase/admin_test.rb +++ b/hbase-shell/src/test/ruby/hbase/admin_test.rb @@ -966,24 +966,24 @@ def teardown assert_match(/12345678/, admin.describe(@test_name)) end - define_test "alter should be able to change EC policy" do - command(:alter, @test_name, METHOD => 'table_att', 'ERASURE_CODING_POLICY' => "XOR-2-1-1024k") + define_test 'alter should be able to change EC policy' do + command(:alter, @test_name, METHOD => 'table_att', 'ERASURE_CODING_POLICY' => 'XOR-2-1-1024k') assert_match(/XOR-2-1-1024k/, admin.describe(@test_name)) end - define_test "alter should be able to remove EC policy" do - command(:alter, @test_name, METHOD => 'table_att', 'ERASURE_CODING_POLICY' => "XOR-2-1-1024k") + define_test 'alter should be able to remove EC policy' do + command(:alter, @test_name, METHOD => 'table_att', 'ERASURE_CODING_POLICY' => 'XOR-2-1-1024k') command(:alter, @test_name, METHOD => 'table_att_unset', NAME => 'ERASURE_CODING_POLICY') assert_not_match(/ERASURE_CODING_POLICY/, admin.describe(@test_name)) end - define_test "alter should be able to change EC POLICY w/o table_att" do - command(:alter, @test_name, 'ERASURE_CODING_POLICY' => "XOR-2-1-1024k") + define_test 'alter should be able to change EC POLICY w/o table_att' do + command(:alter, @test_name, 'ERASURE_CODING_POLICY' => 'XOR-2-1-1024k') assert_match(/XOR-2-1-1024k/, admin.describe(@test_name)) end - define_test "alter should be able to remove EC POLICY w/o table_att" do - command(:alter, @test_name, 'ERASURE_CODING_POLICY' => "XOR-2-1-1024k") + define_test 'alter should be able to remove EC POLICY w/o table_att' do + command(:alter, @test_name, 'ERASURE_CODING_POLICY' => 'XOR-2-1-1024k') command(:alter, @test_name, 'ERASURE_CODING_POLICY' => nil) assert_not_match(/ERASURE_CODING_POLICY/, admin.describe(@test_name)) end