-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-25549 Provide a switch that allows avoiding reopening all regions when modifying a table to prevent RIT storms. #2924
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
Changes from 8 commits
9855a6c
8ab8c60
7c78397
0f4a95c
7d031c7
03226ad
e808de3
24c8879
00b8cf6
5cc50ab
4fb025b
3569f87
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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; | ||
|
|
@@ -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 | ||
|
|
@@ -77,14 +80,22 @@ 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 { | ||
| this(env, newTableDescriptor, latch, oldTableDescriptor, shouldCheckDescriptor, true); | ||
| } | ||
|
|
||
| public ModifyTableProcedure(final MasterProcedureEnv env, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since this class is IA.Private, it'd be preferable to not add a new constructor overload unless absolutely necessary. Are there few enough callers that you could simply update one of the existing constructors?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the new constructor I added before and altered the existing constructor instead. |
||
| final TableDescriptor newTableDescriptor, final ProcedurePrepareLatch latch, | ||
| 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 */); | ||
|
|
@@ -104,6 +115,55 @@ 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 ( | ||
| 0 != this.unmodifiedTableDescriptor.getTableName() | ||
| .compareTo(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"); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private boolean isTablePropertyModified(TableDescriptor oldDescriptor, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: javadoc would be great
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
|
@@ -125,7 +185,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); | ||
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)) { | ||
|
|
@@ -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)) { | ||
|
|
@@ -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 | ||
|
|
@@ -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 = | ||
|
|
||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 intomodifyTable(..[five parameters].., boolean reopenRegions). So now, you can see thatmodifyTable(..[five parameters]..)callsmodifyTable(..[five parameters].., boolean reopenRegions)withreopenRegions=true.