saintstack commented on a change in pull request #2497:
URL: https://github.com/apache/hbase/pull/2497#discussion_r501299847
##########
File path:
hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto
##########
@@ -72,6 +72,8 @@ enum ModifyTableState {
MODIFY_TABLE_DELETE_FS_LAYOUT = 5;
MODIFY_TABLE_POST_OPERATION = 6;
MODIFY_TABLE_REOPEN_ALL_REGIONS = 7;
+ MODIFY_TABLE_CLOSE_EXCESS_REPLICAS = 8;
+ MODIFY_TABLE_ASSIGN_NEW_REPLICAS = 9;
Review comment:
Think this could go back to branch-2? For 2.4.0?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java
##########
@@ -870,32 +870,49 @@ public TransitRegionStateProcedure
createOneUnassignProcedure(RegionInfo ri, boo
.sorted(AssignmentManager::compare).toArray(TransitRegionStateProcedure[]::new);
}
+ // for creating unassign TRSP when disabling a table or closing excess
region replicas
+ private TransitRegionStateProcedure
forceCreateUnssignProcedure(RegionStateNode regionNode) {
+ regionNode.lock();
+ try {
+ if (!regionStates.include(regionNode, false) ||
+ regionStates.isRegionOffline(regionNode.getRegionInfo())) {
+ return null;
+ }
+ // As in DisableTableProcedure or ModifyTableProcedure, we will hold the
xlock for table, so
+ // we can make sure that this procedure has not been executed yet, as
TRSP will hold the
+ // shared lock for table all the time. So here we will unset it and when
it is actually
+ // executed, it will find that the attach procedure is not itself and
quit immediately.
+ if (regionNode.getProcedure() != null) {
Review comment:
I don't remember how this worked. It is a technique to stop the current
procedure on the regionNode from running? What is the procedure that is on the
regionNode? Is it something we've put in place? Something expected? Or just a
random procedure that might be running? I don't remember this part of the code.
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
##########
@@ -1846,11 +1846,9 @@ public static void modifyTableSync(Admin admin,
TableDescriptor desc)
*/
public static void setReplicas(Admin admin, TableName table, int
replicaCount)
throws IOException, InterruptedException {
- admin.disableTable(table);
TableDescriptor desc =
TableDescriptorBuilder.newBuilder(admin.getDescriptor(table))
.setRegionReplication(replicaCount).build();
admin.modifyTable(desc);
- admin.enableTable(table);
}
Review comment:
Nice
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/EnableTableProcedure.java
##########
@@ -111,25 +104,16 @@ protected Flow executeFromState(final MasterProcedureEnv
env, final EnableTableS
// How many replicas do we currently have? Check regions returned
from
// in-memory state.
int currentMaxReplica = getMaxReplicaId(regionsOfTable);
-
- // Read the META table to know the number of replicas the table
currently has.
- // If there was a table modification on region replica count then
need to
- // adjust replica counts here.
- int replicasFound = TableName.isMetaTableName(this.tableName)?
Review comment:
We didn't actually use this replicasFound that we read from meta?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java
##########
@@ -870,32 +870,49 @@ public TransitRegionStateProcedure
createOneUnassignProcedure(RegionInfo ri, boo
.sorted(AssignmentManager::compare).toArray(TransitRegionStateProcedure[]::new);
}
+ // for creating unassign TRSP when disabling a table or closing excess
region replicas
+ private TransitRegionStateProcedure
forceCreateUnssignProcedure(RegionStateNode regionNode) {
+ regionNode.lock();
+ try {
+ if (!regionStates.include(regionNode, false) ||
+ regionStates.isRegionOffline(regionNode.getRegionInfo())) {
+ return null;
+ }
+ // As in DisableTableProcedure or ModifyTableProcedure, we will hold the
xlock for table, so
+ // we can make sure that this procedure has not been executed yet, as
TRSP will hold the
+ // shared lock for table all the time. So here we will unset it and when
it is actually
+ // executed, it will find that the attach procedure is not itself and
quit immediately.
+ if (regionNode.getProcedure() != null) {
+ regionNode.unsetProcedure(regionNode.getProcedure());
+ }
+ TransitRegionStateProcedure proc = TransitRegionStateProcedure
+ .unassign(getProcedureEnvironment(), regionNode.getRegionInfo());
+ regionNode.setProcedure(proc);
+ return proc;
Review comment:
Why safe the proc to a local variable? Just return result of unassign?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
