From 9855a6c5523e86145636cb4ed254af6209a2c812 Mon Sep 17 00:00:00 2001 From: Zhuoyue Huang Date: Tue, 26 Sep 2023 17:22:29 +0800 Subject: [PATCH 01/12] HBASE-25549 Provide lazy mode when modifying table to avoid RIT storm --- .../org/apache/hadoop/hbase/client/Admin.java | 20 ++- .../hbase/client/AdminOverAsyncAdmin.java | 7 +- .../hadoop/hbase/client/AsyncAdmin.java | 17 ++- .../hadoop/hbase/client/AsyncHBaseAdmin.java | 7 +- .../hbase/client/RawAsyncHBaseAdmin.java | 7 +- .../hadoop/hbase/client/TableDescriptor.java | 20 ++- .../shaded/protobuf/RequestConverter.java | 4 +- .../main/protobuf/server/master/Master.proto | 1 + .../server/master/MasterProcedure.proto | 1 + .../apache/hadoop/hbase/master/HMaster.java | 14 +-- .../hbase/master/MasterRpcServices.java | 3 +- .../hadoop/hbase/master/MasterServices.java | 18 ++- .../procedure/ModifyTableProcedure.java | 75 ++++++++++- .../hbase/master/MockNoopMasterServices.java | 3 +- .../procedure/TestModifyTableProcedure.java | 116 +++++++++++++++++- .../hbase/rsgroup/VerifyingRSGroupAdmin.java | 6 +- hbase-shell/src/main/ruby/hbase/admin.rb | 17 ++- .../src/main/ruby/shell/commands/alter.rb | 7 ++ .../hbase/thrift2/client/ThriftAdmin.java | 4 + 19 files changed, 313 insertions(+), 34 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java index 4d579c16af26..53e44c16abe2 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java @@ -1044,7 +1044,25 @@ default void modifyTable(TableDescriptor td) throws IOException { * @return the result of the async modify. You can use Future.get(long, TimeUnit) to wait on the * operation to complete */ - Future modifyTableAsync(TableDescriptor td) throws IOException; + default Future modifyTableAsync(TableDescriptor td) throws IOException{ + return modifyTableAsync(td, true); + } + + /** + * Same as {@link #modifyTableAsync(TableDescriptor td)}. except {@code lazyMode} will control + * whether user lazy mode to modify a table + * + * @param td description of the table + * @param reopenRegions By default, 'modifyTable' reopens all regions, potentially causing a RIT + * (Region In Transition) storm in large tables. If set to 'false', regions + * will remain unaware of the modification until they are individually + * reopened. Please note that this may temporarily result in configuration + * inconsistencies among regions. + * @return the result of the async modify. You can use Future.get(long, TimeUnit) to wait on the + * operation to complete + * @throws IOException if a remote or network exception occurs + */ + Future modifyTableAsync(TableDescriptor td, boolean reopenRegions) throws IOException; /** * Change the store file tracker of the given table. diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java index 690b6406fd3a..85fe5bd8ae56 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java @@ -492,7 +492,12 @@ public Future splitRegionAsync(byte[] regionName, byte[] splitPoint) throw @Override public Future modifyTableAsync(TableDescriptor td) throws IOException { - return admin.modifyTable(td); + return modifyTableAsync(td, true); + } + + @Override + public Future modifyTableAsync(TableDescriptor td, boolean reopenRegions) throws IOException { + return admin.modifyTable(td, reopenRegions); } @Override diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java index 960982f5e3f1..e0018db7c2e9 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java @@ -198,7 +198,22 @@ CompletableFuture createTable(TableDescriptor desc, byte[] startKey, byte[ * Modify an existing table, more IRB friendly version. * @param desc modified description of the table */ - CompletableFuture modifyTable(TableDescriptor desc); + default CompletableFuture modifyTable(TableDescriptor desc){ + return modifyTable(desc, true); + } + + /** + * Modify an existing table, more IRB friendly version. + * + * @param desc description of the table + * @param reopenRegions By default, 'modifyTable' reopens all regions, potentially causing a RIT + * (Region In Transition) storm in large tables. If set to 'false', regions + * will remain unaware of the modification until they are individually + * reopened. Please note that this may temporarily result in configuration + * inconsistencies among regions. + */ + CompletableFuture modifyTable(TableDescriptor desc, boolean reopenRegions); + /** * Change the store file tracker of the given table. diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncHBaseAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncHBaseAdmin.java index 5ee8a6ab8269..1a0f27d86b3f 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncHBaseAdmin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncHBaseAdmin.java @@ -153,7 +153,12 @@ public CompletableFuture createTable(TableDescriptor desc, byte[][] splitK @Override public CompletableFuture modifyTable(TableDescriptor desc) { - return wrap(rawAdmin.modifyTable(desc)); + return modifyTable(desc, true); + } + + @Override + public CompletableFuture modifyTable(TableDescriptor desc, boolean reopenRegions) { + return wrap(rawAdmin.modifyTable(desc, reopenRegions)); } @Override diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java index ee1dfac16bd3..7e8ce957cc8a 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java @@ -692,9 +692,14 @@ private CompletableFuture createTable(TableName tableName, CreateTableRequ @Override public CompletableFuture modifyTable(TableDescriptor desc) { + return modifyTable(desc, true); + } + + @Override + public CompletableFuture modifyTable(TableDescriptor desc, boolean reopenRegions) { return this. procedureCall(desc.getTableName(), RequestConverter.buildModifyTableRequest(desc.getTableName(), desc, ng.getNonceGroup(), - ng.newNonce()), + ng.newNonce(), reopenRegions), (s, c, req, done) -> s.modifyTable(c, req, done), (resp) -> resp.getProcId(), new ModifyTableProcedureBiConsumer(this, desc.getTableName())); } 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..cb243be18e1d 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 @@ -53,9 +53,25 @@ public interface TableDescriptor { if (result != 0) { return result; } + result = getColumnFamilyComparator(cfComparator).compare(lhs, rhs); + if (result != 0){ + return result; + } + // punt on comparison for ordering, just calculate difference + return Integer.compare(lhs.getValues().hashCode(), rhs.getValues().hashCode()); + }; + } + + /** + * This comparator only compare ColumnFamilyDescriptor between two tables + */ + static Comparator + getColumnFamilyComparator( + Comparator cfComparator) { + return (TableDescriptor lhs, TableDescriptor rhs) -> { Collection lhsFamilies = Arrays.asList(lhs.getColumnFamilies()); Collection rhsFamilies = Arrays.asList(rhs.getColumnFamilies()); - result = Integer.compare(lhsFamilies.size(), rhsFamilies.size()); + int result = Integer.compare(lhsFamilies.size(), rhsFamilies.size()); if (result != 0) { return result; } @@ -68,7 +84,7 @@ public interface TableDescriptor { } } // punt on comparison for ordering, just calculate difference - return Integer.compare(lhs.getValues().hashCode(), rhs.getValues().hashCode()); + return 0; }; } diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java index 33884158da48..58376b0c20a9 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java @@ -1097,12 +1097,14 @@ public static CreateTableRequest buildCreateTableRequest(final TableDescriptor t * @return a ModifyTableRequest */ public static ModifyTableRequest buildModifyTableRequest(final TableName tableName, - final TableDescriptor tableDesc, final long nonceGroup, final long nonce) { + final TableDescriptor tableDesc, final long nonceGroup, final long nonce, + final boolean reopenRegions) { ModifyTableRequest.Builder builder = ModifyTableRequest.newBuilder(); builder.setTableName(ProtobufUtil.toProtoTableName(tableName)); builder.setTableSchema(ProtobufUtil.toTableSchema(tableDesc)); builder.setNonceGroup(nonceGroup); builder.setNonce(nonce); + builder.setReopenRegions(reopenRegions); return builder.build(); } diff --git a/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto b/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto index 5d715fdcdd16..e8fb00320e66 100644 --- a/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto +++ b/hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto @@ -194,6 +194,7 @@ message ModifyTableRequest { required TableSchema table_schema = 2; optional uint64 nonce_group = 3 [default = 0]; optional uint64 nonce = 4 [default = 0]; + optional bool reopen_regions = 5 [default = true]; } message ModifyTableResponse { 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 3f3ecd63b002..715d1dbbd4b4 100644 --- a/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto +++ b/hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto @@ -82,6 +82,7 @@ message ModifyTableStateData { required TableSchema modified_table_schema = 3; required bool delete_column_family_in_modify = 4; optional bool should_check_descriptor = 5; + optional bool reopen_regions = 6; } enum TruncateTableState { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index 995bff17724e..ba7f560d0f1a 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -2585,7 +2585,7 @@ public TableDescriptor get() throws IOException { return TableDescriptorBuilder.newBuilder(old).setColumnFamily(column).build(); } - }, nonceGroup, nonce, true); + }, nonceGroup, nonce, true, true); } /** @@ -2612,7 +2612,7 @@ public TableDescriptor get() throws IOException { return TableDescriptorBuilder.newBuilder(old).modifyColumnFamily(descriptor).build(); } - }, nonceGroup, nonce, true); + }, nonceGroup, nonce, true, true); } @Override @@ -2663,7 +2663,7 @@ public TableDescriptor get() throws IOException { } return TableDescriptorBuilder.newBuilder(old).removeColumnFamily(columnName).build(); } - }, nonceGroup, nonce, true); + }, nonceGroup, nonce, true, true); } @Override @@ -2762,7 +2762,7 @@ protected String getDescription() { private long modifyTable(final TableName tableName, final TableDescriptorGetter newDescriptorGetter, final long nonceGroup, final long nonce, - final boolean shouldCheckDescriptor) throws IOException { + final boolean shouldCheckDescriptor, final boolean reopenRegions) throws IOException { return MasterProcedureUtil .submitProcedure(new MasterProcedureUtil.NonceProcedureRunnable(this, nonceGroup, nonce) { @Override @@ -2781,7 +2781,7 @@ protected void run() throws IOException { // checks. This will block only the beginning of the procedure. See HBASE-19953. ProcedurePrepareLatch latch = ProcedurePrepareLatch.createBlockingLatch(); submitProcedure(new ModifyTableProcedure(procedureExecutor.getEnvironment(), - newDescriptor, latch, oldDescriptor, shouldCheckDescriptor)); + newDescriptor, latch, oldDescriptor, shouldCheckDescriptor, reopenRegions)); latch.await(); getMaster().getMasterCoprocessorHost().postModifyTable(tableName, oldDescriptor, @@ -2798,14 +2798,14 @@ protected String getDescription() { @Override public long modifyTable(final TableName tableName, final TableDescriptor newDescriptor, - final long nonceGroup, final long nonce) throws IOException { + final long nonceGroup, final long nonce, final boolean reopenRegions) throws IOException { checkInitialized(); return modifyTable(tableName, new TableDescriptorGetter() { @Override public TableDescriptor get() throws IOException { return newDescriptor; } - }, nonceGroup, nonce, false); + }, nonceGroup, nonce, false, reopenRegions); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java index b6a17d8503b2..ec653a8d263d 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java @@ -1530,7 +1530,8 @@ public ModifyTableResponse modifyTable(RpcController controller, ModifyTableRequ throws ServiceException { try { long procId = server.modifyTable(ProtobufUtil.toTableName(req.getTableName()), - ProtobufUtil.toTableDescriptor(req.getTableSchema()), req.getNonceGroup(), req.getNonce()); + ProtobufUtil.toTableDescriptor(req.getTableSchema()), req.getNonceGroup(), + req.getNonce(), req.getReopenRegions()); return ModifyTableResponse.newBuilder().setProcId(procId).build(); } catch (IOException ioe) { throw new ServiceException(ioe); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java index 933bf0d18150..572a96137b0c 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java @@ -158,8 +158,10 @@ public long truncateTable(final TableName tableName, final boolean preserveSplit * @param tableName The table name * @param descriptor The updated table descriptor */ - long modifyTable(final TableName tableName, final TableDescriptor descriptor, - final long nonceGroup, final long nonce) throws IOException; + default long modifyTable(final TableName tableName, final TableDescriptor descriptor, + final long nonceGroup, final long nonce) throws IOException{ + return modifyTable(tableName, descriptor, nonceGroup, nonce, true); + } /** * Modify the store file tracker of an existing table @@ -167,6 +169,18 @@ long modifyTable(final TableName tableName, final TableDescriptor descriptor, long modifyTableStoreFileTracker(final TableName tableName, final String dstSFT, final long nonceGroup, final long nonce) throws IOException; + /** + * Modify the descriptor of an existing table + * @param tableName The table name + * @param descriptor The updated table descriptor + * @param nonceGroup + * @param nonce + * @param reopenRegions Whether to reopen regions after modifying the table descriptor + * @throws IOException + */ + long modifyTable(final TableName tableName, final TableDescriptor descriptor, + final long nonceGroup, final long nonce, final boolean reopenRegions) throws IOException; + /** * Enable an existing table * @param tableName The table name 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 28f955126bcd..1d913a1c31eb 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 @@ -20,6 +20,7 @@ import java.io.IOException; import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Set; import java.util.function.Supplier; @@ -31,9 +32,11 @@ import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.TableNotFoundException; +import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor; import org.apache.hadoop.hbase.client.RegionInfo; import org.apache.hadoop.hbase.client.RegionReplicaUtil; import org.apache.hadoop.hbase.client.TableDescriptor; +import org.apache.hadoop.hbase.client.TableDescriptorBuilder; import org.apache.hadoop.hbase.master.MasterCoprocessorHost; import org.apache.hadoop.hbase.master.zksyncer.MetaLocationSyncer; import org.apache.hadoop.hbase.procedure2.ProcedureStateSerializer; @@ -56,6 +59,7 @@ public class ModifyTableProcedure extends AbstractStateMachineTableProcedure s = new HashSet<>( + Arrays.asList( + TableDescriptorBuilder.REGION_REPLICATION, + TableDescriptorBuilder.REGION_MEMSTORE_REPLICATION, + RSGroupInfo.TABLE_DESC_PROP_GROUP)); + for (String k : s) { + if (isTablePropertyModified(this.unmodifiedTableDescriptor, this.modifiedTableDescriptor, k)) { + throw new HBaseIOException( + "Can not modify " + k + " of a table when modification won't reopen regions"); + } + } + } + } + + private boolean isTablePropertyModified(TableDescriptor oldDescriptor, TableDescriptor newDescriptor, + String key) { + String oldV = oldDescriptor.getValue(key); + String newV = newDescriptor.getValue(key); + if (oldV == null && newV == null) { + return false; + } else if (oldV != null && newV != null && oldV.equals(newV)) { + return false; + } + return true; } private void initialize(final TableDescriptor unmodifiedTableDescriptor, @@ -125,7 +173,11 @@ protected Flow executeFromState(final MasterProcedureEnv env, final ModifyTableS break; case MODIFY_TABLE_PRE_OPERATION: preModify(env, state); - setNextState(ModifyTableState.MODIFY_TABLE_CLOSE_EXCESS_REPLICAS); + if (reopenRegions){ + setNextState(ModifyTableState.MODIFY_TABLE_CLOSE_EXCESS_REPLICAS); + }else { + setNextState(ModifyTableState.MODIFY_TABLE_UPDATE_TABLE_DESCRIPTOR); + } break; case MODIFY_TABLE_CLOSE_EXCESS_REPLICAS: if (isTableEnabled(env)) { @@ -135,7 +187,11 @@ protected Flow executeFromState(final MasterProcedureEnv env, final ModifyTableS break; case MODIFY_TABLE_UPDATE_TABLE_DESCRIPTOR: updateTableDescriptor(env); - setNextState(ModifyTableState.MODIFY_TABLE_REMOVE_REPLICA_COLUMN); + if(reopenRegions){ + setNextState(ModifyTableState.MODIFY_TABLE_REMOVE_REPLICA_COLUMN); + }else { + setNextState(ModifyTableState.MODIFY_TABLE_POST_OPERATION); + } break; case MODIFY_TABLE_REMOVE_REPLICA_COLUMN: removeReplicaColumnsIfNeeded(env); @@ -143,7 +199,12 @@ protected Flow executeFromState(final MasterProcedureEnv env, final ModifyTableS break; case MODIFY_TABLE_POST_OPERATION: postModify(env, state); - setNextState(ModifyTableState.MODIFY_TABLE_REOPEN_ALL_REGIONS); + if (reopenRegions){ + setNextState(ModifyTableState.MODIFY_TABLE_REOPEN_ALL_REGIONS); + } + else { + return Flow.NO_MORE_STATE; + } break; case MODIFY_TABLE_REOPEN_ALL_REGIONS: if (isTableEnabled(env)) { @@ -238,7 +299,8 @@ protected void serializeStateData(ProcedureStateSerializer serializer) throws IO .setUserInfo(MasterProcedureUtil.toProtoUserInfo(getUser())) .setModifiedTableSchema(ProtobufUtil.toTableSchema(modifiedTableDescriptor)) .setDeleteColumnFamilyInModify(deleteColumnFamilyInModify) - .setShouldCheckDescriptor(shouldCheckDescriptor); + .setShouldCheckDescriptor(shouldCheckDescriptor) + .setReopenRegions(reopenRegions); if (unmodifiedTableDescriptor != null) { modifyTableMsg @@ -260,6 +322,7 @@ protected void deserializeStateData(ProcedureStateSerializer serializer) throws deleteColumnFamilyInModify = modifyTableMsg.getDeleteColumnFamilyInModify(); shouldCheckDescriptor = modifyTableMsg.hasShouldCheckDescriptor() ? modifyTableMsg.getShouldCheckDescriptor() : false; + reopenRegions = modifyTableMsg.hasReopenRegions() ? modifyTableMsg.getReopenRegions() : true; if (modifyTableMsg.hasUnmodifiedTableSchema()) { unmodifiedTableDescriptor = diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java index a19b6ffbec64..67d59f3fea83 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java @@ -258,7 +258,8 @@ public long modifyTable(final TableName tableName, final TableDescriptor descrip } @Override - public long enableTable(final TableName tableName, final long nonceGroup, final long nonce) + public long enableTable(final TableName tableName, final long nonceGroup, final long nonce, + final boolean reopenRegions) throws IOException { return -1; } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java index 51be6385319f..df09f62bfa74 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java @@ -17,26 +17,25 @@ */ package org.apache.hadoop.hbase.master.procedure; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; - -import java.io.IOException; import org.apache.hadoop.hbase.ConcurrentTableModificationException; import org.apache.hadoop.hbase.DoNotRetryIOException; import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.HBaseIOException; import org.apache.hadoop.hbase.InvalidFamilyOperationException; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor; import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder; +import org.apache.hadoop.hbase.client.CoprocessorDescriptorBuilder; import org.apache.hadoop.hbase.client.PerClientRandomNonceGenerator; import org.apache.hadoop.hbase.client.RegionInfo; import org.apache.hadoop.hbase.client.TableDescriptor; import org.apache.hadoop.hbase.client.TableDescriptorBuilder; +import org.apache.hadoop.hbase.io.compress.Compression; import org.apache.hadoop.hbase.master.procedure.MasterProcedureTestingUtility.StepHook; import org.apache.hadoop.hbase.procedure2.Procedure; import org.apache.hadoop.hbase.procedure2.ProcedureExecutor; import org.apache.hadoop.hbase.procedure2.ProcedureTestingUtility; +import org.apache.hadoop.hbase.regionserver.HRegion; import org.apache.hadoop.hbase.testclassification.LargeTests; import org.apache.hadoop.hbase.testclassification.MasterTests; import org.apache.hadoop.hbase.util.Bytes; @@ -49,6 +48,14 @@ import org.junit.experimental.categories.Category; import org.junit.rules.TestName; +import java.io.IOException; +import java.util.List; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + + @Category({ MasterTests.class, LargeTests.class }) public class TestModifyTableProcedure extends TestTableDDLProcedureBase { @@ -584,4 +591,103 @@ public void run() { t2.join(); assertFalse("Expected ConcurrentTableModificationException.", (t1.exception || t2.exception)); } + + @Test + public void testModifyWillNotReopenRegions() throws IOException { + final boolean reopenRegions = false; + final TableName tableName = TableName.valueOf(name.getMethodName()); + final ProcedureExecutor procExec = getMasterProcedureExecutor(); + + MasterProcedureTestingUtility.createTable(procExec, tableName, null, "cf"); + UTIL.getAdmin().disableTable(tableName); + + // Test 1: Modify table without reopening any regions + TableDescriptor htd = UTIL.getAdmin().getDescriptor(tableName); + TableDescriptor modifiedDescriptor = TableDescriptorBuilder.newBuilder(htd) + .setValue("test.hbase.conf", + "test.hbase.conf.value") + .build(); + long procId1 = ProcedureTestingUtility.submitAndWait(procExec, + new ModifyTableProcedure(procExec.getEnvironment(), modifiedDescriptor, null, htd, + false, reopenRegions)); + ProcedureTestingUtility.assertProcNotFailed(procExec.getResult(procId1)); + TableDescriptor currentHtd = UTIL.getAdmin().getDescriptor(tableName); + assertEquals("test.hbase.conf.value", currentHtd.getValue("test.hbase.conf")); + List onlineRegions = UTIL.getHBaseCluster().getRegions(tableName); + //Regions should not aware of any changes. + for (HRegion r : onlineRegions) { + Assert.assertNull(r.getTableDescriptor().getValue("test.hbase.conf")); + } + //Force regions to reopen + for (HRegion r : onlineRegions) { + getMaster().getAssignmentManager().move(r.getRegionInfo()); + } + //After the regions reopen, ensure that the configuration is updated. + for (HRegion r : onlineRegions) { + assertEquals("test.hbase.conf.value", + r.getTableDescriptor().getValue("test.hbase.conf.value")); + } + + // Test 2: Modifying region replication is not allowed + htd = UTIL.getAdmin().getDescriptor(tableName); + long oldRegionReplication = htd.getRegionReplication(); + modifiedDescriptor = TableDescriptorBuilder.newBuilder(htd).setRegionReplication(3).build(); + try { + new ModifyTableProcedure(procExec.getEnvironment(), modifiedDescriptor, null, htd, false, + reopenRegions); + Assert.fail( + "An exception should have been thrown while modifying region replication properties."); + } catch (HBaseIOException e) { + assertTrue(e.getMessage().contains("Can not modify")); + } + currentHtd = UTIL.getAdmin().getDescriptor(tableName); + //Nothing changed + assertEquals(oldRegionReplication, currentHtd.getRegionReplication()); + + // Test 3: Adding CFs is not allowed + htd = UTIL.getAdmin().getDescriptor(tableName); + modifiedDescriptor = TableDescriptorBuilder.newBuilder(htd).setColumnFamily( + ColumnFamilyDescriptorBuilder.newBuilder("NewCF".getBytes()).build()).build(); + try { + new ModifyTableProcedure(procExec.getEnvironment(), modifiedDescriptor, null, htd, false, + reopenRegions); + Assert.fail("Should have thrown an exception while modifying CF!"); + } catch (HBaseIOException e) { + assertTrue(e.getMessage().contains("Cannot modify column families")); + } + currentHtd = UTIL.getAdmin().getDescriptor(tableName); + Assert.assertNull(currentHtd.getColumnFamily("NewCF".getBytes())); + + // Test 4: Modifying CF property is not allowed + htd = UTIL.getAdmin().getDescriptor(tableName); + modifiedDescriptor = TableDescriptorBuilder.newBuilder(htd).modifyColumnFamily( + ColumnFamilyDescriptorBuilder.newBuilder("cf".getBytes()) + .setCompressionType(Compression.Algorithm.SNAPPY).build()) + .build(); + try { + new ModifyTableProcedure(procExec.getEnvironment(), modifiedDescriptor, null, htd, false, + reopenRegions); + Assert.fail("Should have thrown an exception while modifying CF!"); + } catch (HBaseIOException e) { + assertTrue(e.getMessage().contains("Cannot modify column families")); + } + currentHtd = UTIL.getAdmin().getDescriptor(tableName); + assertEquals(Compression.Algorithm.NONE, + currentHtd.getColumnFamily("cf".getBytes()).getCompressionType()); + + // Test 5: Modifying coprocessor is not allowed + htd = UTIL.getAdmin().getDescriptor(tableName); + modifiedDescriptor = TableDescriptorBuilder.newBuilder(htd).setCoprocessor( + CoprocessorDescriptorBuilder.newBuilder("any.coprocessor.name").setJarPath("fake/path") + .build()).build(); + try { + new ModifyTableProcedure(procExec.getEnvironment(), modifiedDescriptor, null, htd, false, + reopenRegions); + Assert.fail("Should have thrown an exception while modifying coprocessor!"); + } catch (HBaseIOException e) { + assertTrue(e.getMessage().contains("Can not modify Coprocessor")); + } + currentHtd = UTIL.getAdmin().getDescriptor(tableName); + assertEquals(0, currentHtd.getCoprocessorDescriptors().size()); + } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java index 9b1d8524d003..35da92dffbf7 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java @@ -413,7 +413,11 @@ public Future splitRegionAsync(byte[] regionName, byte[] splitPoint) throw } public Future modifyTableAsync(TableDescriptor td) throws IOException { - return admin.modifyTableAsync(td); + return modifyTableAsync(td, true); + } + + public Future modifyTableAsync(TableDescriptor td, boolean reopenRegions) throws IOException { + return admin.modifyTableAsync(td, reopenRegions); } public void shutdown() throws IOException { diff --git a/hbase-shell/src/main/ruby/hbase/admin.rb b/hbase-shell/src/main/ruby/hbase/admin.rb index 7477b8ec164f..2f7921f74f85 100644 --- a/hbase-shell/src/main/ruby/hbase/admin.rb +++ b/hbase-shell/src/main/ruby/hbase/admin.rb @@ -774,6 +774,7 @@ def alter(table_name_str, wait = true, *args) # Get table descriptor tdb = TableDescriptorBuilder.newBuilder(@admin.getDescriptor(table_name)) hasTableUpdate = false + reopen_regions = true # Process all args args.each do |arg| @@ -786,6 +787,12 @@ def alter(table_name_str, wait = true, *args) # There are 3 possible options. # 1) Column family spec. Distinguished by having a NAME and no METHOD. method = arg.delete(METHOD) + + if !method.nil? && method == 'no_reopen_regions' + reopen_regions = false; + method = nil + end + if method.nil? && arg.key?(NAME) descriptor = cfd(arg, tdb) column_name = descriptor.getNameAsString @@ -906,9 +913,13 @@ def alter(table_name_str, wait = true, *args) # Bulk apply all table modifications. if hasTableUpdate - future = @admin.modifyTableAsync(tdb.build) - - if wait == true + future = @admin.modifyTableAsync(tdb.build, reopen_regions) + if reopen_regions == false + puts("WARNING: You are using 'no_reopen_regions' to modify a table, which will result in + inconsistencies in the configuration of online regions and other risks. If you encounter + any issues, use the original 'alter' command to make the modification again!") + future.get + elsif wait == true puts 'Updating all regions with the new schema...' future.get end diff --git a/hbase-shell/src/main/ruby/shell/commands/alter.rb b/hbase-shell/src/main/ruby/shell/commands/alter.rb index ad0cb5a5a49b..9ddba5aa95b9 100644 --- a/hbase-shell/src/main/ruby/shell/commands/alter.rb +++ b/hbase-shell/src/main/ruby/shell/commands/alter.rb @@ -72,6 +72,13 @@ def help hbase> alter 't1', CONFIGURATION => {'hbase.hregion.scan.loadColumnFamiliesOnDemand' => 'true'} hbase> alter 't1', {NAME => 'f2', CONFIGURATION => {'hbase.hstore.blockingStoreFiles' => '10'}} +You can also set configuration setting with 'no_reopen_regions' to avoid regions RIT, which let the +modification take effect after regions was reopened (Be careful, the regions of the table may be +configured inconsistently If regions are not reopened after the modification) + + hbase> alter 't1', METHOD => 'no_reopen_regions', CONFIGURATION => {'hbase.hregion.scan + .loadColumnFamiliesOnDemand' => 'true'} + You can also unset configuration settings specific to this table: hbase> alter 't1', METHOD => 'table_conf_unset', NAME => 'hbase.hregion.majorcompaction' diff --git a/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java b/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java index 1b7b6938524a..a86a1f326dbc 100644 --- a/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java +++ b/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java @@ -735,6 +735,10 @@ public Future modifyTableAsync(TableDescriptor td) { throw new NotImplementedException("modifyTableAsync not supported in ThriftAdmin"); } + @Override public Future modifyTableAsync(TableDescriptor td, boolean reopenRegions){ + throw new NotImplementedException("modifyTableAsync not supported in ThriftAdmin"); + } + @Override public void shutdown() { throw new NotImplementedException("shutdown not supported in ThriftAdmin"); From 8ab8c6013c0ef27f8e81dc2da4a8f17c93ff356d Mon Sep 17 00:00:00 2001 From: Zhuoyue Huang Date: Tue, 26 Sep 2023 18:19:27 +0800 Subject: [PATCH 02/12] Some formatting and comment adjustments --- .../org/apache/hadoop/hbase/client/Admin.java | 4 +-- .../hadoop/hbase/client/TableDescriptor.java | 2 +- .../apache/hadoop/hbase/master/HMaster.java | 13 +++++++-- .../hadoop/hbase/master/MasterServices.java | 14 ++++----- .../procedure/ModifyTableProcedure.java | 7 +++++ .../hbase/master/MockNoopMasterServices.java | 9 ++++-- .../procedure/TestModifyTableProcedure.java | 29 ++++++++----------- 7 files changed, 46 insertions(+), 32 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java index 53e44c16abe2..0a0e1bbf5aa9 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java @@ -1049,8 +1049,8 @@ default Future modifyTableAsync(TableDescriptor td) throws IOException{ } /** - * Same as {@link #modifyTableAsync(TableDescriptor td)}. except {@code lazyMode} will control - * whether user lazy mode to modify a table + * The same as {@link #modifyTableAsync(TableDescriptor td)}, except for the reopenRegions + * parameter, which controls whether the process of modifying the table should reopen all regions. * * @param td description of the table * @param reopenRegions By default, 'modifyTable' reopens all regions, potentially causing a RIT 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 cb243be18e1d..b1d7ed51e55e 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 @@ -63,7 +63,7 @@ public interface TableDescriptor { } /** - * This comparator only compare ColumnFamilyDescriptor between two tables + * Check if the ColumnFamilyDescriptors in two tableDescriptors are consistent. */ static Comparator getColumnFamilyComparator( diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index ba7f560d0f1a..757bf7747352 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -2585,7 +2585,7 @@ public TableDescriptor get() throws IOException { return TableDescriptorBuilder.newBuilder(old).setColumnFamily(column).build(); } - }, nonceGroup, nonce, true, true); + }, nonceGroup, nonce, true); } /** @@ -2612,7 +2612,7 @@ public TableDescriptor get() throws IOException { return TableDescriptorBuilder.newBuilder(old).modifyColumnFamily(descriptor).build(); } - }, nonceGroup, nonce, true, true); + }, nonceGroup, nonce, true); } @Override @@ -2663,7 +2663,7 @@ public TableDescriptor get() throws IOException { } return TableDescriptorBuilder.newBuilder(old).removeColumnFamily(columnName).build(); } - }, nonceGroup, nonce, true, true); + }, nonceGroup, nonce, true); } @Override @@ -2760,6 +2760,13 @@ protected String getDescription() { }); } + private long modifyTable(final TableName tableName, + final TableDescriptorGetter newDescriptorGetter, final long nonceGroup, final long nonce, + final boolean shouldCheckDescriptor) throws IOException { + return modifyTable(tableName, newDescriptorGetter, nonceGroup, nonce, shouldCheckDescriptor, + true); + } + private long modifyTable(final TableName tableName, final TableDescriptorGetter newDescriptorGetter, final long nonceGroup, final long nonce, final boolean shouldCheckDescriptor, final boolean reopenRegions) throws IOException { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java index 572a96137b0c..e1b5d7adc6a7 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java @@ -159,16 +159,10 @@ public long truncateTable(final TableName tableName, final boolean preserveSplit * @param descriptor The updated table descriptor */ default long modifyTable(final TableName tableName, final TableDescriptor descriptor, - final long nonceGroup, final long nonce) throws IOException{ + final long nonceGroup, final long nonce) throws IOException { return modifyTable(tableName, descriptor, nonceGroup, nonce, true); } - /** - * Modify the store file tracker of an existing table - */ - long modifyTableStoreFileTracker(final TableName tableName, final String dstSFT, - final long nonceGroup, final long nonce) throws IOException; - /** * Modify the descriptor of an existing table * @param tableName The table name @@ -181,6 +175,12 @@ long modifyTableStoreFileTracker(final TableName tableName, final String dstSFT, long modifyTable(final TableName tableName, final TableDescriptor descriptor, final long nonceGroup, final long nonce, final boolean reopenRegions) throws IOException; + /** + * Modify the store file tracker of an existing table + */ + long modifyTableStoreFileTracker(final TableName tableName, final String dstSFT, + final long nonceGroup, final long nonce) throws IOException; + /** * Enable an existing table * @param tableName The table name 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 1d913a1c31eb..f0786e846f85 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 @@ -84,6 +84,13 @@ public ModifyTableProcedure(final MasterProcedureEnv env, final TableDescriptor this(env, htd, latch, null, false, true); } + public ModifyTableProcedure(final MasterProcedureEnv env, + final TableDescriptor newTableDescriptor, final ProcedurePrepareLatch latch, + final TableDescriptor oldTableDescriptor, final boolean shouldCheckDescriptor) + throws HBaseIOException { + this(env, newTableDescriptor, latch, oldTableDescriptor, shouldCheckDescriptor, true); + } + public ModifyTableProcedure(final MasterProcedureEnv env, final TableDescriptor newTableDescriptor, final ProcedurePrepareLatch latch, final TableDescriptor oldTableDescriptor, final boolean shouldCheckDescriptor, diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java index 67d59f3fea83..84b3f70fffa0 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java @@ -258,8 +258,13 @@ public long modifyTable(final TableName tableName, final TableDescriptor descrip } @Override - public long enableTable(final TableName tableName, final long nonceGroup, final long nonce, - final boolean reopenRegions) + public long modifyTable(TableName tableName, TableDescriptor descriptor, long nonceGroup, + long nonce, boolean reopenRegions) throws IOException { + return modifyTable(tableName, descriptor, nonceGroup, nonce, reopenRegions); + } + + @Override + public long enableTable(final TableName tableName, final long nonceGroup, final long nonce) throws IOException { return -1; } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java index df09f62bfa74..06078f218f87 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java @@ -17,6 +17,12 @@ */ package org.apache.hadoop.hbase.master.procedure; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.io.IOException; +import java.util.List; import org.apache.hadoop.hbase.ConcurrentTableModificationException; import org.apache.hadoop.hbase.DoNotRetryIOException; import org.apache.hadoop.hbase.HBaseClassTestRule; @@ -48,14 +54,6 @@ import org.junit.experimental.categories.Category; import org.junit.rules.TestName; -import java.io.IOException; -import java.util.List; - -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; - - @Category({ MasterTests.class, LargeTests.class }) public class TestModifyTableProcedure extends TestTableDDLProcedureBase { @@ -603,10 +601,8 @@ public void testModifyWillNotReopenRegions() throws IOException { // Test 1: Modify table without reopening any regions TableDescriptor htd = UTIL.getAdmin().getDescriptor(tableName); - TableDescriptor modifiedDescriptor = TableDescriptorBuilder.newBuilder(htd) - .setValue("test.hbase.conf", - "test.hbase.conf.value") - .build(); + TableDescriptor modifiedDescriptor = TableDescriptorBuilder.newBuilder(htd).setValue("test" + + ".hbase.conf", "test.hbase.conf.value").build(); long procId1 = ProcedureTestingUtility.submitAndWait(procExec, new ModifyTableProcedure(procExec.getEnvironment(), modifiedDescriptor, null, htd, false, reopenRegions)); @@ -614,15 +610,15 @@ public void testModifyWillNotReopenRegions() throws IOException { TableDescriptor currentHtd = UTIL.getAdmin().getDescriptor(tableName); assertEquals("test.hbase.conf.value", currentHtd.getValue("test.hbase.conf")); List onlineRegions = UTIL.getHBaseCluster().getRegions(tableName); - //Regions should not aware of any changes. + // Regions should not aware of any changes. for (HRegion r : onlineRegions) { Assert.assertNull(r.getTableDescriptor().getValue("test.hbase.conf")); } - //Force regions to reopen + // Force regions to reopen for (HRegion r : onlineRegions) { getMaster().getAssignmentManager().move(r.getRegionInfo()); } - //After the regions reopen, ensure that the configuration is updated. + // After the regions reopen, ensure that the configuration is updated. for (HRegion r : onlineRegions) { assertEquals("test.hbase.conf.value", r.getTableDescriptor().getValue("test.hbase.conf.value")); @@ -662,8 +658,7 @@ public void testModifyWillNotReopenRegions() throws IOException { htd = UTIL.getAdmin().getDescriptor(tableName); modifiedDescriptor = TableDescriptorBuilder.newBuilder(htd).modifyColumnFamily( ColumnFamilyDescriptorBuilder.newBuilder("cf".getBytes()) - .setCompressionType(Compression.Algorithm.SNAPPY).build()) - .build(); + .setCompressionType(Compression.Algorithm.SNAPPY).build()).build(); try { new ModifyTableProcedure(procExec.getEnvironment(), modifiedDescriptor, null, htd, false, reopenRegions); From 7c78397ff9eea5c630659dbc73a7843cd3e239b3 Mon Sep 17 00:00:00 2001 From: Zhuoyue Huang Date: Thu, 28 Sep 2023 10:45:51 +0800 Subject: [PATCH 03/12] Fixed the issue causing unit tests to fail --- .../hadoop/hbase/master/procedure/ModifyTableProcedure.java | 1 + .../org/apache/hadoop/hbase/master/MockNoopMasterServices.java | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) 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 f0786e846f85..e7319257810b 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 @@ -97,6 +97,7 @@ public ModifyTableProcedure(final MasterProcedureEnv env, final boolean reopenRegions) throws HBaseIOException { super(env, latch); + this.reopenRegions = reopenRegions; initialize(oldTableDescriptor, shouldCheckDescriptor); this.modifiedTableDescriptor = newTableDescriptor; preflightChecks(env, null/* No table checks; if changing peers, table can be online */); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java index 84b3f70fffa0..34d2f7dc253e 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java @@ -260,7 +260,7 @@ public long modifyTable(final TableName tableName, final TableDescriptor descrip @Override public long modifyTable(TableName tableName, TableDescriptor descriptor, long nonceGroup, long nonce, boolean reopenRegions) throws IOException { - return modifyTable(tableName, descriptor, nonceGroup, nonce, reopenRegions); + return -1; } @Override From 0f4a95c59e980125bb6af881d234996498985e53 Mon Sep 17 00:00:00 2001 From: Huangzhuoyue Date: Wed, 4 Oct 2023 10:59:28 +0800 Subject: [PATCH 04/12] Modify the code based on the suggested comments --- .../hbase/master/procedure/TestModifyTableProcedure.java | 1 - hbase-shell/src/main/ruby/hbase/admin.rb | 4 ++-- hbase-shell/src/main/ruby/shell/commands/alter.rb | 4 ++-- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java index 06078f218f87..c12625502b1b 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java @@ -597,7 +597,6 @@ public void testModifyWillNotReopenRegions() throws IOException { final ProcedureExecutor procExec = getMasterProcedureExecutor(); MasterProcedureTestingUtility.createTable(procExec, tableName, null, "cf"); - UTIL.getAdmin().disableTable(tableName); // Test 1: Modify table without reopening any regions TableDescriptor htd = UTIL.getAdmin().getDescriptor(tableName); diff --git a/hbase-shell/src/main/ruby/hbase/admin.rb b/hbase-shell/src/main/ruby/hbase/admin.rb index 2f7921f74f85..2c80728237f7 100644 --- a/hbase-shell/src/main/ruby/hbase/admin.rb +++ b/hbase-shell/src/main/ruby/hbase/admin.rb @@ -788,7 +788,7 @@ def alter(table_name_str, wait = true, *args) # 1) Column family spec. Distinguished by having a NAME and no METHOD. method = arg.delete(METHOD) - if !method.nil? && method == 'no_reopen_regions' + if !method.nil? && method == 'avoid_reopening_regions' reopen_regions = false; method = nil end @@ -915,7 +915,7 @@ def alter(table_name_str, wait = true, *args) if hasTableUpdate future = @admin.modifyTableAsync(tdb.build, reopen_regions) if reopen_regions == false - puts("WARNING: You are using 'no_reopen_regions' to modify a table, which will result in + puts("WARNING: You are using 'avoid_reopening_regions' to modify a table, which will result in inconsistencies in the configuration of online regions and other risks. If you encounter any issues, use the original 'alter' command to make the modification again!") future.get diff --git a/hbase-shell/src/main/ruby/shell/commands/alter.rb b/hbase-shell/src/main/ruby/shell/commands/alter.rb index 9ddba5aa95b9..2e278cd82d63 100644 --- a/hbase-shell/src/main/ruby/shell/commands/alter.rb +++ b/hbase-shell/src/main/ruby/shell/commands/alter.rb @@ -72,11 +72,11 @@ def help hbase> alter 't1', CONFIGURATION => {'hbase.hregion.scan.loadColumnFamiliesOnDemand' => 'true'} hbase> alter 't1', {NAME => 'f2', CONFIGURATION => {'hbase.hstore.blockingStoreFiles' => '10'}} -You can also set configuration setting with 'no_reopen_regions' to avoid regions RIT, which let the +You can also set configuration setting with 'avoid_reopening_regions' to avoid regions RIT, which let the modification take effect after regions was reopened (Be careful, the regions of the table may be configured inconsistently If regions are not reopened after the modification) - hbase> alter 't1', METHOD => 'no_reopen_regions', CONFIGURATION => {'hbase.hregion.scan + hbase> alter 't1', METHOD => 'avoid_reopening_regions', CONFIGURATION => {'hbase.hregion.scan .loadColumnFamiliesOnDemand' => 'true'} You can also unset configuration settings specific to this table: From 7d031c787b2daba8ef7939b6452cbe55c7fb22e9 Mon Sep 17 00:00:00 2001 From: Zhuoyue Huang Date: Sat, 7 Oct 2023 14:37:42 +0800 Subject: [PATCH 05/12] Fixed unit tests and enabled modification of column family properties. --- .../org/apache/hadoop/hbase/client/Admin.java | 5 +- .../hbase/client/AdminOverAsyncAdmin.java | 3 +- .../hadoop/hbase/client/AsyncAdmin.java | 4 +- .../hadoop/hbase/client/TableDescriptor.java | 5 +- .../shaded/protobuf/RequestConverter.java | 2 +- .../apache/hadoop/hbase/master/HMaster.java | 6 +- .../hbase/master/MasterRpcServices.java | 4 +- .../hadoop/hbase/master/MasterServices.java | 9 +- .../procedure/ModifyTableProcedure.java | 72 ++++++++-------- .../hbase/master/MockNoopMasterServices.java | 2 +- .../procedure/TestModifyTableProcedure.java | 83 ++++++++++--------- .../hbase/rsgroup/VerifyingRSGroupAdmin.java | 3 +- .../hbase/thrift2/client/ThriftAdmin.java | 3 +- pom.xml | 2 + 14 files changed, 104 insertions(+), 99 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java index 0a0e1bbf5aa9..161e11ec3a8b 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java @@ -1044,14 +1044,13 @@ default void modifyTable(TableDescriptor td) throws IOException { * @return the result of the async modify. You can use Future.get(long, TimeUnit) to wait on the * operation to complete */ - default Future modifyTableAsync(TableDescriptor td) throws IOException{ + default Future modifyTableAsync(TableDescriptor td) throws IOException { return modifyTableAsync(td, true); } /** * The same as {@link #modifyTableAsync(TableDescriptor td)}, except for the reopenRegions * parameter, which controls whether the process of modifying the table should reopen all regions. - * * @param td description of the table * @param reopenRegions By default, 'modifyTable' reopens all regions, potentially causing a RIT * (Region In Transition) storm in large tables. If set to 'false', regions @@ -1059,7 +1058,7 @@ default Future modifyTableAsync(TableDescriptor td) throws IOException{ * reopened. Please note that this may temporarily result in configuration * inconsistencies among regions. * @return the result of the async modify. You can use Future.get(long, TimeUnit) to wait on the - * operation to complete + * operation to complete * @throws IOException if a remote or network exception occurs */ Future modifyTableAsync(TableDescriptor td, boolean reopenRegions) throws IOException; diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java index 85fe5bd8ae56..ad7b98413c94 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java @@ -496,7 +496,8 @@ public Future modifyTableAsync(TableDescriptor td) throws IOException { } @Override - public Future modifyTableAsync(TableDescriptor td, boolean reopenRegions) throws IOException { + public Future modifyTableAsync(TableDescriptor td, boolean reopenRegions) + throws IOException { return admin.modifyTable(td, reopenRegions); } diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java index e0018db7c2e9..44d8b6b5e999 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java @@ -198,13 +198,12 @@ CompletableFuture createTable(TableDescriptor desc, byte[] startKey, byte[ * Modify an existing table, more IRB friendly version. * @param desc modified description of the table */ - default CompletableFuture modifyTable(TableDescriptor desc){ + default CompletableFuture modifyTable(TableDescriptor desc) { return modifyTable(desc, true); } /** * Modify an existing table, more IRB friendly version. - * * @param desc description of the table * @param reopenRegions By default, 'modifyTable' reopens all regions, potentially causing a RIT * (Region In Transition) storm in large tables. If set to 'false', regions @@ -214,7 +213,6 @@ default CompletableFuture modifyTable(TableDescriptor desc){ */ CompletableFuture modifyTable(TableDescriptor desc, boolean reopenRegions); - /** * Change the store file tracker of the given table. * @param tableName the table you want to change 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 b1d7ed51e55e..7aa72a2347b6 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 @@ -54,7 +54,7 @@ public interface TableDescriptor { return result; } result = getColumnFamilyComparator(cfComparator).compare(lhs, rhs); - if (result != 0){ + if (result != 0) { return result; } // punt on comparison for ordering, just calculate difference @@ -66,8 +66,7 @@ public interface TableDescriptor { * Check if the ColumnFamilyDescriptors in two tableDescriptors are consistent. */ static Comparator - getColumnFamilyComparator( - Comparator cfComparator) { + getColumnFamilyComparator(Comparator cfComparator) { return (TableDescriptor lhs, TableDescriptor rhs) -> { Collection lhsFamilies = Arrays.asList(lhs.getColumnFamilies()); Collection rhsFamilies = Arrays.asList(rhs.getColumnFamilies()); diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java index 58376b0c20a9..66feb67e4412 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java @@ -1098,7 +1098,7 @@ public static CreateTableRequest buildCreateTableRequest(final TableDescriptor t */ public static ModifyTableRequest buildModifyTableRequest(final TableName tableName, final TableDescriptor tableDesc, final long nonceGroup, final long nonce, - final boolean reopenRegions) { + final boolean reopenRegions) { ModifyTableRequest.Builder builder = ModifyTableRequest.newBuilder(); builder.setTableName(ProtobufUtil.toProtoTableName(tableName)); builder.setTableSchema(ProtobufUtil.toTableSchema(tableDesc)); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index 757bf7747352..9b83d01dab0f 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -2761,10 +2761,10 @@ protected String getDescription() { } private long modifyTable(final TableName tableName, - final TableDescriptorGetter newDescriptorGetter, final long nonceGroup, final long nonce, - final boolean shouldCheckDescriptor) throws IOException { + final TableDescriptorGetter newDescriptorGetter, final long nonceGroup, final long nonce, + final boolean shouldCheckDescriptor) throws IOException { return modifyTable(tableName, newDescriptorGetter, nonceGroup, nonce, shouldCheckDescriptor, - true); + true); } private long modifyTable(final TableName tableName, diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java index ec653a8d263d..5c28a3527573 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java @@ -1530,8 +1530,8 @@ public ModifyTableResponse modifyTable(RpcController controller, ModifyTableRequ throws ServiceException { try { long procId = server.modifyTable(ProtobufUtil.toTableName(req.getTableName()), - ProtobufUtil.toTableDescriptor(req.getTableSchema()), req.getNonceGroup(), - req.getNonce(), req.getReopenRegions()); + ProtobufUtil.toTableDescriptor(req.getTableSchema()), req.getNonceGroup(), req.getNonce(), + req.getReopenRegions()); return ModifyTableResponse.newBuilder().setProcId(procId).build(); } catch (IOException ioe) { throw new ServiceException(ioe); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java index e1b5d7adc6a7..d8cae2279d53 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterServices.java @@ -165,15 +165,12 @@ default long modifyTable(final TableName tableName, final TableDescriptor descri /** * Modify the descriptor of an existing table - * @param tableName The table name - * @param descriptor The updated table descriptor - * @param nonceGroup - * @param nonce + * @param tableName The table name + * @param descriptor The updated table descriptor * @param reopenRegions Whether to reopen regions after modifying the table descriptor - * @throws IOException */ long modifyTable(final TableName tableName, final TableDescriptor descriptor, - final long nonceGroup, final long nonce, final boolean reopenRegions) throws IOException; + final long nonceGroup, final long nonce, final boolean reopenRegions) throws IOException; /** * Modify the store file tracker of an existing table 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 e7319257810b..344421f0dc81 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 @@ -32,7 +32,6 @@ import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.TableNotFoundException; -import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor; import org.apache.hadoop.hbase.client.RegionInfo; import org.apache.hadoop.hbase.client.RegionReplicaUtil; import org.apache.hadoop.hbase.client.TableDescriptor; @@ -85,17 +84,16 @@ public ModifyTableProcedure(final MasterProcedureEnv env, final TableDescriptor } public ModifyTableProcedure(final MasterProcedureEnv env, - final TableDescriptor newTableDescriptor, final ProcedurePrepareLatch latch, - final TableDescriptor oldTableDescriptor, final boolean shouldCheckDescriptor) - throws HBaseIOException { + final TableDescriptor newTableDescriptor, final ProcedurePrepareLatch latch, + final TableDescriptor oldTableDescriptor, final boolean shouldCheckDescriptor) + throws HBaseIOException { this(env, newTableDescriptor, latch, oldTableDescriptor, shouldCheckDescriptor, true); } public ModifyTableProcedure(final MasterProcedureEnv env, final TableDescriptor newTableDescriptor, final ProcedurePrepareLatch latch, final TableDescriptor oldTableDescriptor, final boolean shouldCheckDescriptor, - final boolean reopenRegions) - throws HBaseIOException { + final boolean reopenRegions) throws HBaseIOException { super(env, latch); this.reopenRegions = reopenRegions; initialize(oldTableDescriptor, shouldCheckDescriptor); @@ -122,36 +120,42 @@ protected void preflightChecks(MasterProcedureEnv env, Boolean enabled) throws H throw new HBaseIOException( "unmodifiedTableDescriptor cannot be null when this table modification won't reopen regions"); } - if (0 != this.unmodifiedTableDescriptor.getTableName() - .compareTo(this.modifiedTableDescriptor.getTableName())) { - throw new HBaseIOException("Cannot change the table name when this modification won't " + - "reopen regions."); + if ( + 0 != this.unmodifiedTableDescriptor.getTableName() + .compareTo(this.modifiedTableDescriptor.getTableName()) + ) { + throw new HBaseIOException( + "Cannot change the table name when this modification won't " + "reopen regions."); } - if (0 != TableDescriptor.getColumnFamilyComparator(ColumnFamilyDescriptor.COMPARATOR) - .compare(this.unmodifiedTableDescriptor, this.modifiedTableDescriptor)){ - throw new HBaseIOException("Cannot modify column families when this modification won't " + - "reopen regions."); + if ( + this.unmodifiedTableDescriptor.getColumnFamilyCount() + != this.modifiedTableDescriptor.getColumnFamilyCount() + ) { + throw new HBaseIOException( + "Cannot add or remove column families when this modification " + "won't reopen regions."); } - if (this.unmodifiedTableDescriptor.getCoprocessorDescriptors().hashCode() - != this.modifiedTableDescriptor.getCoprocessorDescriptors().hashCode()) { - throw new HBaseIOException("Can not modify Coprocessor when table modification won't reopen regions"); + if ( + this.unmodifiedTableDescriptor.getCoprocessorDescriptors().hashCode() + != this.modifiedTableDescriptor.getCoprocessorDescriptors().hashCode() + ) { + throw new HBaseIOException( + "Can not modify Coprocessor when table modification won't reopen regions"); } - final Set s = new HashSet<>( - Arrays.asList( - TableDescriptorBuilder.REGION_REPLICATION, - TableDescriptorBuilder.REGION_MEMSTORE_REPLICATION, - RSGroupInfo.TABLE_DESC_PROP_GROUP)); + final Set s = new HashSet<>(Arrays.asList(TableDescriptorBuilder.REGION_REPLICATION, + TableDescriptorBuilder.REGION_MEMSTORE_REPLICATION, RSGroupInfo.TABLE_DESC_PROP_GROUP)); for (String k : s) { - if (isTablePropertyModified(this.unmodifiedTableDescriptor, this.modifiedTableDescriptor, k)) { + if ( + isTablePropertyModified(this.unmodifiedTableDescriptor, this.modifiedTableDescriptor, k) + ) { throw new HBaseIOException( - "Can not modify " + k + " of a table when modification won't reopen regions"); + "Can not modify " + k + " of a table when modification won't reopen regions"); } } } } - private boolean isTablePropertyModified(TableDescriptor oldDescriptor, TableDescriptor newDescriptor, - String key) { + private boolean isTablePropertyModified(TableDescriptor oldDescriptor, + TableDescriptor newDescriptor, String key) { String oldV = oldDescriptor.getValue(key); String newV = newDescriptor.getValue(key); if (oldV == null && newV == null) { @@ -181,9 +185,9 @@ protected Flow executeFromState(final MasterProcedureEnv env, final ModifyTableS break; case MODIFY_TABLE_PRE_OPERATION: preModify(env, state); - if (reopenRegions){ + if (reopenRegions) { setNextState(ModifyTableState.MODIFY_TABLE_CLOSE_EXCESS_REPLICAS); - }else { + } else { setNextState(ModifyTableState.MODIFY_TABLE_UPDATE_TABLE_DESCRIPTOR); } break; @@ -195,9 +199,9 @@ protected Flow executeFromState(final MasterProcedureEnv env, final ModifyTableS break; case MODIFY_TABLE_UPDATE_TABLE_DESCRIPTOR: updateTableDescriptor(env); - if(reopenRegions){ + if (reopenRegions) { setNextState(ModifyTableState.MODIFY_TABLE_REMOVE_REPLICA_COLUMN); - }else { + } else { setNextState(ModifyTableState.MODIFY_TABLE_POST_OPERATION); } break; @@ -207,10 +211,9 @@ protected Flow executeFromState(final MasterProcedureEnv env, final ModifyTableS break; case MODIFY_TABLE_POST_OPERATION: postModify(env, state); - if (reopenRegions){ + if (reopenRegions) { setNextState(ModifyTableState.MODIFY_TABLE_REOPEN_ALL_REGIONS); - } - else { + } else { return Flow.NO_MORE_STATE; } break; @@ -307,8 +310,7 @@ protected void serializeStateData(ProcedureStateSerializer serializer) throws IO .setUserInfo(MasterProcedureUtil.toProtoUserInfo(getUser())) .setModifiedTableSchema(ProtobufUtil.toTableSchema(modifiedTableDescriptor)) .setDeleteColumnFamilyInModify(deleteColumnFamilyInModify) - .setShouldCheckDescriptor(shouldCheckDescriptor) - .setReopenRegions(reopenRegions); + .setShouldCheckDescriptor(shouldCheckDescriptor).setReopenRegions(reopenRegions); if (unmodifiedTableDescriptor != null) { modifyTableMsg diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java index 34d2f7dc253e..f37fd86807a7 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/MockNoopMasterServices.java @@ -259,7 +259,7 @@ public long modifyTable(final TableName tableName, final TableDescriptor descrip @Override public long modifyTable(TableName tableName, TableDescriptor descriptor, long nonceGroup, - long nonce, boolean reopenRegions) throws IOException { + long nonce, boolean reopenRegions) throws IOException { return -1; } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java index c12625502b1b..97b3a62311f2 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java @@ -22,7 +22,6 @@ import static org.junit.Assert.assertTrue; import java.io.IOException; -import java.util.List; import org.apache.hadoop.hbase.ConcurrentTableModificationException; import org.apache.hadoop.hbase.DoNotRetryIOException; import org.apache.hadoop.hbase.HBaseClassTestRule; @@ -600,27 +599,27 @@ public void testModifyWillNotReopenRegions() throws IOException { // Test 1: Modify table without reopening any regions TableDescriptor htd = UTIL.getAdmin().getDescriptor(tableName); - TableDescriptor modifiedDescriptor = TableDescriptorBuilder.newBuilder(htd).setValue("test" + - ".hbase.conf", "test.hbase.conf.value").build(); - long procId1 = ProcedureTestingUtility.submitAndWait(procExec, - new ModifyTableProcedure(procExec.getEnvironment(), modifiedDescriptor, null, htd, - false, reopenRegions)); + TableDescriptor modifiedDescriptor = TableDescriptorBuilder.newBuilder(htd) + .setValue("test" + ".hbase.conf", "test.hbase.conf.value").build(); + long procId1 = ProcedureTestingUtility.submitAndWait(procExec, new ModifyTableProcedure( + procExec.getEnvironment(), modifiedDescriptor, null, htd, false, reopenRegions)); ProcedureTestingUtility.assertProcNotFailed(procExec.getResult(procId1)); TableDescriptor currentHtd = UTIL.getAdmin().getDescriptor(tableName); assertEquals("test.hbase.conf.value", currentHtd.getValue("test.hbase.conf")); - List onlineRegions = UTIL.getHBaseCluster().getRegions(tableName); + // Regions should not aware of any changes. - for (HRegion r : onlineRegions) { + for (HRegion r : UTIL.getHBaseCluster().getRegions(tableName)) { Assert.assertNull(r.getTableDescriptor().getValue("test.hbase.conf")); } + // Force regions to reopen - for (HRegion r : onlineRegions) { + for (HRegion r : UTIL.getHBaseCluster().getRegions(tableName)) { getMaster().getAssignmentManager().move(r.getRegionInfo()); } + // After the regions reopen, ensure that the configuration is updated. - for (HRegion r : onlineRegions) { - assertEquals("test.hbase.conf.value", - r.getTableDescriptor().getValue("test.hbase.conf.value")); + for (HRegion r : UTIL.getHBaseCluster().getRegions(tableName)) { + assertEquals("test.hbase.conf.value", r.getTableDescriptor().getValue("test.hbase.conf")); } // Test 2: Modifying region replication is not allowed @@ -628,55 +627,61 @@ public void testModifyWillNotReopenRegions() throws IOException { long oldRegionReplication = htd.getRegionReplication(); modifiedDescriptor = TableDescriptorBuilder.newBuilder(htd).setRegionReplication(3).build(); try { - new ModifyTableProcedure(procExec.getEnvironment(), modifiedDescriptor, null, htd, false, - reopenRegions); + ProcedureTestingUtility.submitAndWait(procExec, new ModifyTableProcedure( + procExec.getEnvironment(), modifiedDescriptor, null, htd, false, reopenRegions)); Assert.fail( - "An exception should have been thrown while modifying region replication properties."); + "An exception should have been thrown while modifying region replication properties."); } catch (HBaseIOException e) { assertTrue(e.getMessage().contains("Can not modify")); } currentHtd = UTIL.getAdmin().getDescriptor(tableName); - //Nothing changed + // Nothing changed assertEquals(oldRegionReplication, currentHtd.getRegionReplication()); // Test 3: Adding CFs is not allowed htd = UTIL.getAdmin().getDescriptor(tableName); - modifiedDescriptor = TableDescriptorBuilder.newBuilder(htd).setColumnFamily( - ColumnFamilyDescriptorBuilder.newBuilder("NewCF".getBytes()).build()).build(); + modifiedDescriptor = TableDescriptorBuilder.newBuilder(htd) + .setColumnFamily(ColumnFamilyDescriptorBuilder.newBuilder("NewCF".getBytes()).build()) + .build(); try { - new ModifyTableProcedure(procExec.getEnvironment(), modifiedDescriptor, null, htd, false, - reopenRegions); + ProcedureTestingUtility.submitAndWait(procExec, new ModifyTableProcedure( + procExec.getEnvironment(), modifiedDescriptor, null, htd, false, reopenRegions)); Assert.fail("Should have thrown an exception while modifying CF!"); } catch (HBaseIOException e) { - assertTrue(e.getMessage().contains("Cannot modify column families")); + assertTrue(e.getMessage().contains("Cannot add or remove column families")); } currentHtd = UTIL.getAdmin().getDescriptor(tableName); Assert.assertNull(currentHtd.getColumnFamily("NewCF".getBytes())); - // Test 4: Modifying CF property is not allowed + // Test 4: Modifying CF property is allowed htd = UTIL.getAdmin().getDescriptor(tableName); - modifiedDescriptor = TableDescriptorBuilder.newBuilder(htd).modifyColumnFamily( - ColumnFamilyDescriptorBuilder.newBuilder("cf".getBytes()) - .setCompressionType(Compression.Algorithm.SNAPPY).build()).build(); - try { - new ModifyTableProcedure(procExec.getEnvironment(), modifiedDescriptor, null, htd, false, - reopenRegions); - Assert.fail("Should have thrown an exception while modifying CF!"); - } catch (HBaseIOException e) { - assertTrue(e.getMessage().contains("Cannot modify column families")); + modifiedDescriptor = + TableDescriptorBuilder + .newBuilder(htd).modifyColumnFamily(ColumnFamilyDescriptorBuilder + .newBuilder("cf".getBytes()).setCompressionType(Compression.Algorithm.SNAPPY).build()) + .build(); + ProcedureTestingUtility.submitAndWait(procExec, new ModifyTableProcedure( + procExec.getEnvironment(), modifiedDescriptor, null, htd, false, reopenRegions)); + for (HRegion r : UTIL.getHBaseCluster().getRegions(tableName)) { + Assert.assertEquals(Compression.Algorithm.NONE, + r.getTableDescriptor().getColumnFamily("cf".getBytes()).getCompressionType()); + } + for (HRegion r : UTIL.getHBaseCluster().getRegions(tableName)) { + getMaster().getAssignmentManager().move(r.getRegionInfo()); + } + for (HRegion r : UTIL.getHBaseCluster().getRegions(tableName)) { + Assert.assertEquals(Compression.Algorithm.SNAPPY, + r.getTableDescriptor().getColumnFamily("cf".getBytes()).getCompressionType()); } - currentHtd = UTIL.getAdmin().getDescriptor(tableName); - assertEquals(Compression.Algorithm.NONE, - currentHtd.getColumnFamily("cf".getBytes()).getCompressionType()); // Test 5: Modifying coprocessor is not allowed htd = UTIL.getAdmin().getDescriptor(tableName); - modifiedDescriptor = TableDescriptorBuilder.newBuilder(htd).setCoprocessor( - CoprocessorDescriptorBuilder.newBuilder("any.coprocessor.name").setJarPath("fake/path") - .build()).build(); + modifiedDescriptor = + TableDescriptorBuilder.newBuilder(htd).setCoprocessor(CoprocessorDescriptorBuilder + .newBuilder("any.coprocessor.name").setJarPath("fake/path").build()).build(); try { - new ModifyTableProcedure(procExec.getEnvironment(), modifiedDescriptor, null, htd, false, - reopenRegions); + ProcedureTestingUtility.submitAndWait(procExec, new ModifyTableProcedure( + procExec.getEnvironment(), modifiedDescriptor, null, htd, false, reopenRegions)); Assert.fail("Should have thrown an exception while modifying coprocessor!"); } catch (HBaseIOException e) { assertTrue(e.getMessage().contains("Can not modify Coprocessor")); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java index 35da92dffbf7..813d3e121edb 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java @@ -416,7 +416,8 @@ public Future modifyTableAsync(TableDescriptor td) throws IOException { return modifyTableAsync(td, true); } - public Future modifyTableAsync(TableDescriptor td, boolean reopenRegions) throws IOException { + public Future modifyTableAsync(TableDescriptor td, boolean reopenRegions) + throws IOException { return admin.modifyTableAsync(td, reopenRegions); } diff --git a/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java b/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java index a86a1f326dbc..72fc025e9461 100644 --- a/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java +++ b/hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java @@ -735,7 +735,8 @@ public Future modifyTableAsync(TableDescriptor td) { throw new NotImplementedException("modifyTableAsync not supported in ThriftAdmin"); } - @Override public Future modifyTableAsync(TableDescriptor td, boolean reopenRegions){ + @Override + public Future modifyTableAsync(TableDescriptor td, boolean reopenRegions) { throw new NotImplementedException("modifyTableAsync not supported in ThriftAdmin"); } diff --git a/pom.xml b/pom.xml index 56058b8e2b97..66d632209d6e 100644 --- a/pom.xml +++ b/pom.xml @@ -2819,6 +2819,7 @@ **/generated/* **/package-info.java + **/.idea/** From 03226ad0137fbf501a0fc56899ce99fe6eafb1e0 Mon Sep 17 00:00:00 2001 From: Zhuoyue Huang Date: Sun, 8 Oct 2023 10:20:24 +0800 Subject: [PATCH 06/12] Revert some useless changes. --- .../hadoop/hbase/client/TableDescriptor.java | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) 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 7aa72a2347b6..1c91819ac4b9 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 @@ -53,24 +53,9 @@ public interface TableDescriptor { if (result != 0) { return result; } - result = getColumnFamilyComparator(cfComparator).compare(lhs, rhs); - if (result != 0) { - return result; - } - // punt on comparison for ordering, just calculate difference - return Integer.compare(lhs.getValues().hashCode(), rhs.getValues().hashCode()); - }; - } - - /** - * Check if the ColumnFamilyDescriptors in two tableDescriptors are consistent. - */ - static Comparator - getColumnFamilyComparator(Comparator cfComparator) { - return (TableDescriptor lhs, TableDescriptor rhs) -> { Collection lhsFamilies = Arrays.asList(lhs.getColumnFamilies()); Collection rhsFamilies = Arrays.asList(rhs.getColumnFamilies()); - int result = Integer.compare(lhsFamilies.size(), rhsFamilies.size()); + result = Integer.compare(lhsFamilies.size(), rhsFamilies.size()); if (result != 0) { return result; } @@ -83,7 +68,7 @@ public interface TableDescriptor { } } // punt on comparison for ordering, just calculate difference - return 0; + return Integer.compare(lhs.getValues().hashCode(), rhs.getValues().hashCode()); }; } From e808de38edbaf68dac2cb755bdde335fb6edc0da Mon Sep 17 00:00:00 2001 From: Zhuoyue Huang Date: Mon, 9 Oct 2023 18:54:17 +0800 Subject: [PATCH 07/12] use 'REOPEN_REGIONS' instead of METHOD=>'avoid_reopening_regions' --- hbase-shell/src/main/ruby/hbase/admin.rb | 20 ++++++++++--------- hbase-shell/src/main/ruby/hbase_constants.rb | 1 + .../src/main/ruby/shell/commands/alter.rb | 8 ++++---- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/hbase-shell/src/main/ruby/hbase/admin.rb b/hbase-shell/src/main/ruby/hbase/admin.rb index 2c80728237f7..4695b277b849 100644 --- a/hbase-shell/src/main/ruby/hbase/admin.rb +++ b/hbase-shell/src/main/ruby/hbase/admin.rb @@ -784,15 +784,17 @@ def alter(table_name_str, wait = true, *args) # Normalize args to support shortcut delete syntax arg = { METHOD => 'delete', NAME => arg['delete'] } if arg['delete'] + if arg.key?(REOPEN_REGIONS) + if !['true', 'false'].include?(arg[REOPEN_REGIONS].downcase) + raise(ArgumentError, "Invalid 'REOPEN_REGIONS' for non-boolean value.") + end + reopen_regions = JBoolean.valueOf(arg[REOPEN_REGIONS]) + arg.delete(REOPEN_REGIONS) + end + # There are 3 possible options. # 1) Column family spec. Distinguished by having a NAME and no METHOD. method = arg.delete(METHOD) - - if !method.nil? && method == 'avoid_reopening_regions' - reopen_regions = false; - method = nil - end - if method.nil? && arg.key?(NAME) descriptor = cfd(arg, tdb) column_name = descriptor.getNameAsString @@ -915,9 +917,9 @@ def alter(table_name_str, wait = true, *args) if hasTableUpdate future = @admin.modifyTableAsync(tdb.build, reopen_regions) if reopen_regions == false - puts("WARNING: You are using 'avoid_reopening_regions' to modify a table, which will result in - inconsistencies in the configuration of online regions and other risks. If you encounter - any issues, use the original 'alter' command to make the modification again!") + puts("WARNING: You are using REOPEN_REGIONS => 'false' to modify a table, which will + result in inconsistencies in the configuration of online regions and other risks. If you + encounter any issues, use the original 'alter' command to make the modification again!") future.get elsif wait == true puts 'Updating all regions with the new schema...' diff --git a/hbase-shell/src/main/ruby/hbase_constants.rb b/hbase-shell/src/main/ruby/hbase_constants.rb index 5f994c7b5ae0..d4df1f8f5821 100644 --- a/hbase-shell/src/main/ruby/hbase_constants.rb +++ b/hbase-shell/src/main/ruby/hbase_constants.rb @@ -107,6 +107,7 @@ module HBaseConstants VALUE = 'VALUE'.freeze VERSIONS = org.apache.hadoop.hbase.HConstants::VERSIONS VISIBILITY = 'VISIBILITY'.freeze + REOPEN_REGIONS = 'REOPEN_REGIONS'.freeze # aliases ENDKEY = STOPROW diff --git a/hbase-shell/src/main/ruby/shell/commands/alter.rb b/hbase-shell/src/main/ruby/shell/commands/alter.rb index 2e278cd82d63..c0d4f9ffb67e 100644 --- a/hbase-shell/src/main/ruby/shell/commands/alter.rb +++ b/hbase-shell/src/main/ruby/shell/commands/alter.rb @@ -72,11 +72,11 @@ def help hbase> alter 't1', CONFIGURATION => {'hbase.hregion.scan.loadColumnFamiliesOnDemand' => 'true'} hbase> alter 't1', {NAME => 'f2', CONFIGURATION => {'hbase.hstore.blockingStoreFiles' => '10'}} -You can also set configuration setting with 'avoid_reopening_regions' to avoid regions RIT, which let the -modification take effect after regions was reopened (Be careful, the regions of the table may be -configured inconsistently If regions are not reopened after the modification) +You can also set configuration setting with REOPEN_REGIONS=>'false' to avoid regions RIT, which +let the modification take effect after regions was reopened (Be careful, the regions of the table +may be configured inconsistently If regions are not reopened after the modification) - hbase> alter 't1', METHOD => 'avoid_reopening_regions', CONFIGURATION => {'hbase.hregion.scan + hbase> alter 't1', REOPEN_REGIONS => 'false', CONFIGURATION => {'hbase.hregion.scan .loadColumnFamiliesOnDemand' => 'true'} You can also unset configuration settings specific to this table: From 24c88792c23a4766bce0af0a2f718b68f4b8739b Mon Sep 17 00:00:00 2001 From: Zhuoyue Huang Date: Tue, 10 Oct 2023 09:48:21 +0800 Subject: [PATCH 08/12] Some unrelated unite tests are unstable, resubmit to trigger CI --- .../hbase/master/procedure/TestModifyTableProcedure.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java index 97b3a62311f2..5deabbca993e 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java @@ -606,17 +606,14 @@ public void testModifyWillNotReopenRegions() throws IOException { ProcedureTestingUtility.assertProcNotFailed(procExec.getResult(procId1)); TableDescriptor currentHtd = UTIL.getAdmin().getDescriptor(tableName); assertEquals("test.hbase.conf.value", currentHtd.getValue("test.hbase.conf")); - // Regions should not aware of any changes. for (HRegion r : UTIL.getHBaseCluster().getRegions(tableName)) { Assert.assertNull(r.getTableDescriptor().getValue("test.hbase.conf")); } - // Force regions to reopen for (HRegion r : UTIL.getHBaseCluster().getRegions(tableName)) { getMaster().getAssignmentManager().move(r.getRegionInfo()); } - // After the regions reopen, ensure that the configuration is updated. for (HRegion r : UTIL.getHBaseCluster().getRegions(tableName)) { assertEquals("test.hbase.conf.value", r.getTableDescriptor().getValue("test.hbase.conf")); From 00b8cf670a35a8095a2189f384b816feecbbd43e Mon Sep 17 00:00:00 2001 From: Zhuoyue Huang Date: Wed, 1 Nov 2023 09:55:52 +0800 Subject: [PATCH 09/12] slight modifications --- .../org/apache/hadoop/hbase/master/HMaster.java | 2 +- .../master/procedure/ModifyTableProcedure.java | 15 ++++----------- .../procedure/TestModifyTableProcedure.java | 13 +++++++++++++ .../src/main/ruby/shell/commands/alter.rb | 16 +++++++++++++++- 4 files changed, 33 insertions(+), 13 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index 9b83d01dab0f..1964c45f81ca 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -1163,7 +1163,7 @@ private void finishActiveMasterInitialization() throws IOException, InterruptedE procedureExecutor.submitProcedure(new ModifyTableProcedure( procedureExecutor.getEnvironment(), TableDescriptorBuilder.newBuilder(metaDesc) .setRegionReplication(replicasNumInConf).build(), - null, metaDesc, false)); + null, metaDesc, false, 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 344421f0dc81..847ab13a3e97 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 @@ -83,13 +83,6 @@ public ModifyTableProcedure(final MasterProcedureEnv env, final TableDescriptor this(env, htd, latch, null, false, true); } - public ModifyTableProcedure(final MasterProcedureEnv env, - final TableDescriptor newTableDescriptor, final ProcedurePrepareLatch latch, - final TableDescriptor oldTableDescriptor, final boolean shouldCheckDescriptor) - throws HBaseIOException { - this(env, newTableDescriptor, latch, oldTableDescriptor, shouldCheckDescriptor, true); - } - public ModifyTableProcedure(final MasterProcedureEnv env, final TableDescriptor newTableDescriptor, final ProcedurePrepareLatch latch, final TableDescriptor oldTableDescriptor, final boolean shouldCheckDescriptor, @@ -120,10 +113,8 @@ protected void preflightChecks(MasterProcedureEnv env, Boolean enabled) throws H throw new HBaseIOException( "unmodifiedTableDescriptor cannot be null when this table modification won't reopen regions"); } - if ( - 0 != this.unmodifiedTableDescriptor.getTableName() - .compareTo(this.modifiedTableDescriptor.getTableName()) - ) { + if (!this.unmodifiedTableDescriptor.getTableName() + .equals(this.modifiedTableDescriptor.getTableName())) { throw new HBaseIOException( "Cannot change the table name when this modification won't " + "reopen regions."); } @@ -185,6 +176,8 @@ protected Flow executeFromState(final MasterProcedureEnv env, final ModifyTableS break; case MODIFY_TABLE_PRE_OPERATION: preModify(env, state); + // We cannot allow changes to region replicas when 'reopenRegions==false', + // as this mode bypasses the state management required for modifying region replicas. if (reopenRegions) { setNextState(ModifyTableState.MODIFY_TABLE_CLOSE_EXCESS_REPLICAS); } else { diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java index 5deabbca993e..bacfedc962e4 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java @@ -685,5 +685,18 @@ public void testModifyWillNotReopenRegions() throws IOException { } currentHtd = UTIL.getAdmin().getDescriptor(tableName); assertEquals(0, currentHtd.getCoprocessorDescriptors().size()); + + // Test 6: Modifying is not allowed + htd = UTIL.getAdmin().getDescriptor(tableName); + modifiedDescriptor = + TableDescriptorBuilder.newBuilder(htd).setRegionReplication(3).build(); + try { + ProcedureTestingUtility.submitAndWait(procExec, new ModifyTableProcedure( + procExec.getEnvironment(), modifiedDescriptor, null, htd, false, reopenRegions)); + Assert.fail("Should have thrown an exception while modifying coprocessor!"); + } catch (HBaseIOException e) { + System.out.println(e.getMessage()); + assertTrue(e.getMessage().contains("Can not modify REGION_REPLICATION")); + } } } diff --git a/hbase-shell/src/main/ruby/shell/commands/alter.rb b/hbase-shell/src/main/ruby/shell/commands/alter.rb index c0d4f9ffb67e..18ec24be7be6 100644 --- a/hbase-shell/src/main/ruby/shell/commands/alter.rb +++ b/hbase-shell/src/main/ruby/shell/commands/alter.rb @@ -76,8 +76,22 @@ def help let the modification take effect after regions was reopened (Be careful, the regions of the table may be configured inconsistently If regions are not reopened after the modification) + hbase> alter 't1', REOPEN_REGIONS => 'false', MAX_FILESIZE => '134217728' hbase> alter 't1', REOPEN_REGIONS => 'false', CONFIGURATION => {'hbase.hregion.scan - .loadColumnFamiliesOnDemand' => 'true'} + .loadColumnFamiliesOnDemand' => 'true'} + +However, be aware that: +1. Inconsistency Risks: If the regions are not reopened after the modification, the table's regions +may become inconsistently configured. Ensure that you manually reopen the regions as soon as +possible to apply the changes consistently across the entire table. +2. If changes are made to the table without reopening the regions, we currently only allow +lightweight operations. The following types of changes, which may lead to unknown situations, +will throw an exception: + a. Adding or removing CFs, coprocessors. + b. Modifying the table name. + c. Changing region replica related configurations such as 'REGION_REPLICATION' + and 'REGION_MEMSTORE_REPLICATION'. + d. Changing the rsgroup. You can also unset configuration settings specific to this table: From 5cc50abfef0e35b04506706795e6b308e9da3a3c Mon Sep 17 00:00:00 2001 From: Zhuoyue Huang Date: Wed, 1 Nov 2023 18:05:02 +0800 Subject: [PATCH 10/12] spotless apply --- .../hbase/master/procedure/ModifyTableProcedure.java | 6 ++++-- .../hbase/master/procedure/TestModifyTableProcedure.java | 7 +++---- 2 files changed, 7 insertions(+), 6 deletions(-) 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 847ab13a3e97..45c56e006e3c 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 @@ -113,8 +113,10 @@ protected void preflightChecks(MasterProcedureEnv env, Boolean enabled) throws H throw new HBaseIOException( "unmodifiedTableDescriptor cannot be null when this table modification won't reopen regions"); } - if (!this.unmodifiedTableDescriptor.getTableName() - .equals(this.modifiedTableDescriptor.getTableName())) { + if ( + !this.unmodifiedTableDescriptor.getTableName() + .equals(this.modifiedTableDescriptor.getTableName()) + ) { throw new HBaseIOException( "Cannot change the table name when this modification won't " + "reopen regions."); } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java index bacfedc962e4..c8043b8ee3b2 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java @@ -686,13 +686,12 @@ public void testModifyWillNotReopenRegions() throws IOException { currentHtd = UTIL.getAdmin().getDescriptor(tableName); assertEquals(0, currentHtd.getCoprocessorDescriptors().size()); - // Test 6: Modifying is not allowed + // Test 6: Modifying is not allowed htd = UTIL.getAdmin().getDescriptor(tableName); - modifiedDescriptor = - TableDescriptorBuilder.newBuilder(htd).setRegionReplication(3).build(); + modifiedDescriptor = TableDescriptorBuilder.newBuilder(htd).setRegionReplication(3).build(); try { ProcedureTestingUtility.submitAndWait(procExec, new ModifyTableProcedure( - procExec.getEnvironment(), modifiedDescriptor, null, htd, false, reopenRegions)); + procExec.getEnvironment(), modifiedDescriptor, null, htd, false, reopenRegions)); Assert.fail("Should have thrown an exception while modifying coprocessor!"); } catch (HBaseIOException e) { System.out.println(e.getMessage()); From 4fb025b4b97b95279faab857671fb415f642edf5 Mon Sep 17 00:00:00 2001 From: Zhuoyue Huang Date: Wed, 8 Nov 2023 15:53:27 +0800 Subject: [PATCH 11/12] minor revision done --- .../hbase/master/procedure/ModifyTableProcedure.java | 7 +++++++ 1 file changed, 7 insertions(+) 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 45c56e006e3c..56c7c437fdc8 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 @@ -147,6 +147,13 @@ protected void preflightChecks(MasterProcedureEnv env, Boolean enabled) throws H } } + /** + * Comparing the value associated with a given key across two TableDescriptor instances' properties. + * @param oldDescriptor + * @param newDescriptor + * @param key + * @return True if the table property key is the same in both. + * */ private boolean isTablePropertyModified(TableDescriptor oldDescriptor, TableDescriptor newDescriptor, String key) { String oldV = oldDescriptor.getValue(key); From 3569f87127192836d7eb0d01c29f53a3a25239e0 Mon Sep 17 00:00:00 2001 From: Zhuoyue Huang Date: Wed, 8 Nov 2023 17:47:51 +0800 Subject: [PATCH 12/12] spotless apply --- .../hbase/master/procedure/ModifyTableProcedure.java | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) 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 56c7c437fdc8..ff0d7d2cc94b 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 @@ -148,12 +148,10 @@ protected void preflightChecks(MasterProcedureEnv env, Boolean enabled) throws H } /** - * Comparing the value associated with a given key across two TableDescriptor instances' properties. - * @param oldDescriptor - * @param newDescriptor - * @param key + * Comparing the value associated with a given key across two TableDescriptor instances' + * properties. * @return True if the table property key is the same in both. - * */ + */ private boolean isTablePropertyModified(TableDescriptor oldDescriptor, TableDescriptor newDescriptor, String key) { String oldV = oldDescriptor.getValue(key);