-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
9855a6c
HBASE-25549 Provide lazy mode when modifying table to avoid RIT storm
GeorryHuang 8ab8c60
Some formatting and comment adjustments
GeorryHuang 7c78397
Fixed the issue causing unit tests to fail
GeorryHuang 0f4a95c
Modify the code based on the suggested comments
7d031c7
Fixed unit tests and enabled modification of column family properties.
GeorryHuang 03226ad
Revert some useless changes.
GeorryHuang e808de3
use 'REOPEN_REGIONS' instead of METHOD=>'avoid_reopening_regions'
GeorryHuang 24c8879
Some unrelated unite tests are unstable, resubmit to trigger CI
GeorryHuang 00b8cf6
slight modifications
GeorryHuang 5cc50ab
spotless apply
GeorryHuang 4fb025b
minor revision done
GeorryHuang 3569f87
spotless apply
GeorryHuang File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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,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 */); | ||
|
@@ -104,6 +108,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 ( | ||
!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"); | ||
} | ||
} | ||
} | ||
} | ||
|
||
private boolean isTablePropertyModified(TableDescriptor oldDescriptor, | ||
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 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 +178,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)) { | ||
|
@@ -135,15 +194,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 +305,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 +327,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 = | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
.