Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HBASE-25549 Provide a switch that allows avoiding reopening all regions when modifying a table to prevent RIT storms. #2924

Merged
merged 12 commits into from
Nov 9, 2023
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -1044,7 +1044,24 @@ 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<Void> modifyTableAsync(TableDescriptor td) throws IOException;
default Future<Void> 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
* 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<Void> modifyTableAsync(TableDescriptor td, boolean reopenRegions) throws IOException;

/**
* Change the store file tracker of the given table.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,13 @@ public Future<Void> splitRegionAsync(byte[] regionName, byte[] splitPoint) throw

@Override
public Future<Void> modifyTableAsync(TableDescriptor td) throws IOException {
return admin.modifyTable(td);
return modifyTableAsync(td, true);
}

@Override
public Future<Void> modifyTableAsync(TableDescriptor td, boolean reopenRegions)
throws IOException {
return admin.modifyTable(td, reopenRegions);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,20 @@ CompletableFuture<Void> createTable(TableDescriptor desc, byte[] startKey, byte[
* Modify an existing table, more IRB friendly version.
* @param desc modified description of the table
*/
CompletableFuture<Void> modifyTable(TableDescriptor desc);
default CompletableFuture<Void> 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<Void> modifyTable(TableDescriptor desc, boolean reopenRegions);

/**
* Change the store file tracker of the given table.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,12 @@ public CompletableFuture<Void> createTable(TableDescriptor desc, byte[][] splitK

@Override
public CompletableFuture<Void> modifyTable(TableDescriptor desc) {
return wrap(rawAdmin.modifyTable(desc));
return modifyTable(desc, true);
}

@Override
public CompletableFuture<Void> modifyTable(TableDescriptor desc, boolean reopenRegions) {
return wrap(rawAdmin.modifyTable(desc, reopenRegions));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -692,9 +692,14 @@ private CompletableFuture<Void> createTable(TableName tableName, CreateTableRequ

@Override
public CompletableFuture<Void> modifyTable(TableDescriptor desc) {
return modifyTable(desc, true);
}

@Override
public CompletableFuture<Void> modifyTable(TableDescriptor desc, boolean reopenRegions) {
return this.<ModifyTableRequest, ModifyTableResponse> 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()));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
}
Expand Down Expand Up @@ -2763,6 +2763,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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking into why we need this overload. I see this is called from addColumn/deleteColumn/modifyColumn. Does it make sense to add reopenRegion support to modifyColumn at least?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we have two methods:

modifyTable(..[five parameters]..)
modifyTable(..[five parameters].., boolean reopenRegions)

The methods 'addColumn/deleteColumn/modifyColumn' that you mentioned will call method modifyTable(..[five parameters]..) directly as before. They are not aware of the 'reopenRegions' parameter.

To make the code structure cleaner, I only changed one line in method modifyTable(..[five parameters]..), which then turns into modifyTable(..[five parameters].., boolean reopenRegions). So now, you can see that modifyTable(..[five parameters]..) calls modifyTable(..[five parameters].., boolean reopenRegions) with reopenRegions=true.

true);
}

private long modifyTable(final TableName tableName,
final TableDescriptorGetter newDescriptorGetter, final long nonceGroup, final long nonce,
final boolean shouldCheckDescriptor, final boolean reopenRegions) throws IOException {
return MasterProcedureUtil
.submitProcedure(new MasterProcedureUtil.NonceProcedureRunnable(this, nonceGroup, nonce) {
@Override
Expand All @@ -2781,7 +2788,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,
Expand All @@ -2798,14 +2805,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);

}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,19 @@ public long truncateTable(final TableName tableName, final boolean preserveSplit
* @param tableName The table name
* @param descriptor The updated table descriptor
*/
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 descriptor of an existing table
* @param tableName The table name
* @param descriptor The updated table descriptor
* @param reopenRegions Whether to reopen regions after modifying the table descriptor
*/
long modifyTable(final TableName tableName, final TableDescriptor descriptor,
final long nonceGroup, final long nonce) throws IOException;
final long nonceGroup, final long nonce, final boolean reopenRegions) throws IOException;

/**
* Modify the store file tracker of an existing table
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -34,6 +35,7 @@
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;
Expand All @@ -56,6 +58,7 @@ public class ModifyTableProcedure extends AbstractStateMachineTableProcedure<Mod
private TableDescriptor modifiedTableDescriptor;
private boolean deleteColumnFamilyInModify;
private boolean shouldCheckDescriptor;
private boolean reopenRegions;
/**
* List of column families that cannot be deleted from the hbase:meta table. They are critical to
* cluster operation. This is a bit of an odd place to keep this list but then this is the tooling
Expand All @@ -77,14 +80,15 @@ public ModifyTableProcedure(final MasterProcedureEnv env, final TableDescriptor

public ModifyTableProcedure(final MasterProcedureEnv env, final TableDescriptor htd,
final ProcedurePrepareLatch latch) throws HBaseIOException {
this(env, htd, latch, null, false);
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 {
final TableDescriptor oldTableDescriptor, final boolean shouldCheckDescriptor,
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 */);
Expand All @@ -104,6 +108,60 @@ protected void preflightChecks(MasterProcedureEnv env, Boolean enabled) throws H
}
}
}
if (!reopenRegions) {
if (this.unmodifiedTableDescriptor == null) {
throw new HBaseIOException(
"unmodifiedTableDescriptor cannot be null when this table modification won't reopen regions");
}
if (
!this.unmodifiedTableDescriptor.getTableName()
.equals(this.modifiedTableDescriptor.getTableName())
) {
throw new HBaseIOException(
"Cannot change the table name 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");
}
final Set<String> 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");
}
}
}
}

/**
* Comparing the value associated with a given key across two TableDescriptor instances'
* properties.
* @return True if the table property <code>key</code> is the same in both.
*/
private boolean isTablePropertyModified(TableDescriptor oldDescriptor,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: javadoc would be great

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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,
Expand All @@ -125,7 +183,13 @@ protected Flow executeFromState(final MasterProcedureEnv env, final ModifyTableS
break;
case MODIFY_TABLE_PRE_OPERATION:
preModify(env, state);
setNextState(ModifyTableState.MODIFY_TABLE_CLOSE_EXCESS_REPLICAS);
// 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);
bbeaudreault marked this conversation as resolved.
Show resolved Hide resolved
} else {
setNextState(ModifyTableState.MODIFY_TABLE_UPDATE_TABLE_DESCRIPTOR);
}
break;
case MODIFY_TABLE_CLOSE_EXCESS_REPLICAS:
if (isTableEnabled(env)) {
Expand All @@ -135,15 +199,23 @@ 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);
setNextState(ModifyTableState.MODIFY_TABLE_POST_OPERATION);
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)) {
Expand Down Expand Up @@ -238,7 +310,7 @@ 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
Expand All @@ -260,6 +332,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 =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,12 @@ public long modifyTable(final TableName tableName, final TableDescriptor descrip
return -1;
}

@Override
public long modifyTable(TableName tableName, TableDescriptor descriptor, long nonceGroup,
long nonce, boolean reopenRegions) throws IOException {
return -1;
}

@Override
public long enableTable(final TableName tableName, final long nonceGroup, final long nonce)
throws IOException {
Expand Down
Loading