pankaj72981 commented on a change in pull request #2883:
URL: https://github.com/apache/hbase/pull/2883#discussion_r574993242
##########
File path:
hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminEndpoint.java
##########
@@ -508,21 +508,56 @@ public void preCreateTableAction(
if (desc.getTableName().isSystemTable()) {
return;
}
- RSGroupInfo rsGroupInfo =
groupInfoManager.determineRSGroupInfoForTable(desc.getTableName());
+ moveTableToValidRSGroup(desc);
+ }
+
+ @Override
+ public TableDescriptor
preModifyTable(ObserverContext<MasterCoprocessorEnvironment> ctx,
Review comment:
We should override `preModifyTableAction`.
##########
File path:
hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminEndpoint.java
##########
@@ -508,21 +508,56 @@ public void preCreateTableAction(
if (desc.getTableName().isSystemTable()) {
return;
}
- RSGroupInfo rsGroupInfo =
groupInfoManager.determineRSGroupInfoForTable(desc.getTableName());
+ moveTableToValidRSGroup(desc);
+ }
+
+ @Override
+ public TableDescriptor
preModifyTable(ObserverContext<MasterCoprocessorEnvironment> ctx,
+ TableName tableName, TableDescriptor currentDescriptor, TableDescriptor
newDescriptor)
+ throws IOException {
+ if (newDescriptor.getTableName().isSystemTable()) {
+ return newDescriptor;
+ }
+ // If rs group is changed, it must exist
+ moveTableToValidRSGroup(newDescriptor);
+ return newDescriptor;
+ }
+
+ // Determine and validate rs group then move table to this valid rs group.
+ private void moveTableToValidRSGroup(TableDescriptor desc) throws
IOException {
+ RSGroupInfo rsGroupInfo = determineRSGroupInfoForTable(desc);
if (rsGroupInfo == null) {
- throw new ConstraintException("Default RSGroup for this table " +
desc.getTableName()
- + " does not exist.");
+ throw new ConstraintException(
+ "Default RSGroup for this table " + desc.getTableName() + " does not
exist.");
}
if (!RSGroupUtil.rsGroupHasOnlineServer(master, rsGroupInfo)) {
- throw new HBaseIOException("No online servers in the rsgroup " +
rsGroupInfo.getName()
- + " which table " + desc.getTableName().getNameAsString() + " belongs
to");
+ throw new HBaseIOException(
+ "No online servers in the rsgroup " + rsGroupInfo.getName() + " which
table "
+ + desc.getTableName().getNameAsString() + " belongs to");
}
- synchronized (groupInfoManager) {
- groupInfoManager.moveTables(
- Collections.singleton(desc.getTableName()), rsGroupInfo.getName());
+ // In case of modify table, when rs group is not changed, move is not
required.
+ if (!rsGroupInfo.containsTable(desc.getTableName())) {
+ synchronized (groupInfoManager) {
+ groupInfoManager
Review comment:
Currently we aren't doing any rollback if any exception thrown while
updating the table descriptor in FS, so there will be inconsistent info in
groupInfoManager cache.
So we should validate the modified RSGroup existence in
`preModifyTableAction` & groupInfoManager cache update in
`postCompletedModifyTableAction`.
----------------------------------------------------------------
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:
us...@infra.apache.org
