[GitHub] [hbase] pankaj72981 commented on a change in pull request #2883: HBASE-25492: Create table with rsgroup
pankaj72981 commented on a change in pull request #2883:
URL: https://github.com/apache/hbase/pull/2883#discussion_r588208030
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java
##
@@ -594,6 +598,11 @@ public TableDescriptorBuilder setReplicationScope(int
scope) {
return this;
}
+ public TableDescriptorBuilder setRegionServerGroup(String group) {
Review comment:
Add java doc for the public APIs.
Sorry for late comment.
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
[GitHub] [hbase] pankaj72981 commented on a change in pull request #2883: HBASE-25492: Create table with rsgroup
pankaj72981 commented on a change in pull request #2883:
URL: https://github.com/apache/hbase/pull/2883#discussion_r588047976
##
File path:
hbase-rsgroup/src/test/java/org/apache/hadoop/hbase/rsgroup/TestTableDescriptorWithRSGroup.java
##
@@ -0,0 +1,389 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hbase.rsgroup;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import java.io.IOException;
+import java.util.List;
+import java.util.Optional;
+import java.util.regex.Pattern;
+import org.apache.hadoop.hbase.DoNotRetryIOException;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.Waiter;
+import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor;
+import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder;
+import org.apache.hadoop.hbase.client.SnapshotDescription;
+import org.apache.hadoop.hbase.client.TableDescriptor;
+import org.apache.hadoop.hbase.client.TableDescriptorBuilder;
+import org.apache.hadoop.hbase.constraint.ConstraintException;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.apache.hbase.thirdparty.com.google.common.collect.Sets;
+
+@Category({ LargeTests.class }) public class TestTableDescriptorWithRSGroup
Review comment:
Last commit introduced code formatter problems, please correct them.
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
[GitHub] [hbase] pankaj72981 commented on a change in pull request #2883: HBASE-25492: Create table with rsgroup
pankaj72981 commented on a change in pull request #2883:
URL: https://github.com/apache/hbase/pull/2883#discussion_r588047976
##
File path:
hbase-rsgroup/src/test/java/org/apache/hadoop/hbase/rsgroup/TestTableDescriptorWithRSGroup.java
##
@@ -0,0 +1,389 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hbase.rsgroup;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import java.io.IOException;
+import java.util.List;
+import java.util.Optional;
+import java.util.regex.Pattern;
+import org.apache.hadoop.hbase.DoNotRetryIOException;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.Waiter;
+import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor;
+import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder;
+import org.apache.hadoop.hbase.client.SnapshotDescription;
+import org.apache.hadoop.hbase.client.TableDescriptor;
+import org.apache.hadoop.hbase.client.TableDescriptorBuilder;
+import org.apache.hadoop.hbase.constraint.ConstraintException;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.apache.hbase.thirdparty.com.google.common.collect.Sets;
+
+@Category({ LargeTests.class }) public class TestTableDescriptorWithRSGroup
Review comment:
Last commit instroduced code formatter problems, please correct them.
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
[GitHub] [hbase] pankaj72981 commented on a change in pull request #2883: HBASE-25492: Create table with rsgroup
pankaj72981 commented on a change in pull request #2883:
URL: https://github.com/apache/hbase/pull/2883#discussion_r575984981
##
File path:
hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServer.java
##
@@ -718,4 +729,49 @@ private void checkForDeadOrOnlineServers(Set
servers) throws Constraint
}
}
}
+
+ private void moveOrModifyTables(Set tables, RSGroupInfo
targetGroup)
+throws IOException {
+Set tablesToBeMoved = new HashSet<>(tables.size());
+Set tablesToBeModified = new HashSet<>(tables.size());
+for (TableName tableName : tables) {
+ TableDescriptor descriptor = master.getTableDescriptors().get(tableName);
+ if (descriptor == null) {
+LOG.error("TableDescriptor of table {} not found. "
+ + "Skipping the region movement of this table.");
+continue;
+ }
+ if (descriptor.getRegionServerGroup().isPresent()) {
+tablesToBeModified.add(descriptor);
+ } else {
+tablesToBeMoved.add(tableName);
+ }
+}
+if (!tablesToBeMoved.isEmpty()) {
+ moveTableRegionsToGroup(tablesToBeMoved, targetGroup);
Review comment:
moveTableRegionsToGroup is SYNC, modifyTables has to wait until all
regions are moved to desired group successfully in moveTableRegionsToGroup. So
it will impact the overall performance of move table/server rsgroup operation.
https://github.com/apache/hbase/blob/2e31d1d46efa9f04b5d6e8b359bbe7abd9deacc8/hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServer.java#L359
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
[GitHub] [hbase] pankaj72981 commented on a change in pull request #2883: HBASE-25492: Create table with rsgroup
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 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 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
[GitHub] [hbase] pankaj72981 commented on a change in pull request #2883: HBASE-25492: Create table with rsgroup
pankaj72981 commented on a change in pull request #2883:
URL: https://github.com/apache/hbase/pull/2883#discussion_r571978144
##
File path:
hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminEndpoint.java
##
@@ -508,7 +508,7 @@ public void preCreateTableAction(
if (desc.getTableName().isSystemTable()) {
return;
}
-RSGroupInfo rsGroupInfo =
groupInfoManager.determineRSGroupInfoForTable(desc.getTableName());
+RSGroupInfo rsGroupInfo = determineRSGroupInfoForTable(desc);
Review comment:
We should handle the table modify scenario as well when RSGroup is
modified.
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
