Revert "HBASE-22809 Allow creating table in group when rs group contains no live servers (#464)"

This reverts commit 928ecfb443.

hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminEndpoint.java

@@ -21,12 +21,11 @@ import com.google.protobuf.Service;
 import java.io.IOException;
 import java.util.Collections;
 import java.util.List;
-import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
-import java.util.function.Supplier;
 import java.util.stream.Collectors;
 import org.apache.hadoop.hbase.CoprocessorEnvironment;
+import org.apache.hadoop.hbase.HBaseIOException;
 import org.apache.hadoop.hbase.HConstants;
 import org.apache.hadoop.hbase.NamespaceDescriptor;
 import org.apache.hadoop.hbase.ServerName;
@@ -69,7 +68,7 @@ public class RSGroupAdminEndpoint implements MasterCoprocessor, MasterObserver {
     groupInfoManager = RSGroupInfoManagerImpl.getInstance(master);
     groupAdminServer = new RSGroupAdminServer(master, groupInfoManager);
     Class<?> clazz =
-      master.getConfiguration().getClass(HConstants.HBASE_MASTER_LOADBALANCER_CLASS, null);
+        master.getConfiguration().getClass(HConstants.HBASE_MASTER_LOADBALANCER_CLASS, null);
     if (!RSGroupableBalancer.class.isAssignableFrom(clazz)) {
       throw new IOException("Configured balancer does not support RegionServer groups.");
     }
@@ -109,101 +108,85 @@ public class RSGroupAdminEndpoint implements MasterCoprocessor, MasterObserver {
 
   @Override
   public void postClearDeadServers(ObserverContext<MasterCoprocessorEnvironment> ctx,
-    List<ServerName> servers, List<ServerName> notClearedServers) throws IOException {
+      List<ServerName> servers, List<ServerName> notClearedServers) throws IOException {
     Set<Address> clearedServer =
-      servers.stream().filter(server -> !notClearedServers.contains(server))
-        .map(ServerName::getAddress).collect(Collectors.toSet());
+        servers.stream().filter(server -> !notClearedServers.contains(server))
+            .map(ServerName::getAddress).collect(Collectors.toSet());
     if (!clearedServer.isEmpty()) {
       groupAdminServer.removeServers(clearedServer);
     }
   }
 
-  private RSGroupInfo checkGroupExists(Optional<String> optGroupName, Supplier<String> forWhom)
-    throws IOException {
+  private void checkGroupExists(Optional<String> optGroupName) throws IOException {
     if (optGroupName.isPresent()) {
       String groupName = optGroupName.get();
-      RSGroupInfo group = groupAdminServer.getRSGroupInfo(groupName);
-      if (group == null) {
-        throw new ConstraintException(
-          "Region server group " + groupName + " for " + forWhom.get() + " does not exit");
+      if (groupAdminServer.getRSGroupInfo(groupName) == null) {
+        throw new ConstraintException("Region server group " + groupName + " does not exit");
       }
-      return group;
     }
-    return null;
   }
 
-  private Optional<String> getNamespaceGroup(NamespaceDescriptor namespaceDesc) {
-    return Optional
-      .ofNullable(namespaceDesc.getConfigurationValue(RSGroupInfo.NAMESPACE_DESC_PROP_GROUP));
-  }
-
-  // Do not allow creating new tables/namespaces which has an empty rs group, expect the default rs
-  // group. Notice that we do not check for online servers, as this is not stable because region
-  // servers can die at any time.
-  private void checkGroupNotEmpty(RSGroupInfo rsGroupInfo, Supplier<String> forWhom)
-    throws ConstraintException {
-    if (rsGroupInfo == null || rsGroupInfo.getName().equals(RSGroupInfo.DEFAULT_GROUP)) {
-      // we do not have a rs group config or we explicitly set the rs group to default, then no need
-      // to check.
-      return;
-    }
-    if (rsGroupInfo.getServers().isEmpty()) {
-      throw new ConstraintException(
-        "No servers in the rsgroup " + rsGroupInfo.getName() + " for " + forWhom.get());
+  private boolean rsgroupHasServersOnline(TableDescriptor desc) throws IOException {
+    RSGroupInfo rsGroupInfo;
+    Optional<String> optGroupName = desc.getRegionServerGroup();
+    if (optGroupName.isPresent()) {
+      String groupName = optGroupName.get();
+      if (groupName.equals(RSGroupInfo.DEFAULT_GROUP)) {
+        // do not check for default group
+        return true;
+      }
+      rsGroupInfo = groupAdminServer.getRSGroupInfo(groupName);
+      if (rsGroupInfo == null) {
+        throw new ConstraintException(
+            "RSGroup " + groupName + " for table " + desc.getTableName() + " does not exist");
+      }
+    } else {
+      NamespaceDescriptor nd =
+          master.getClusterSchema().getNamespace(desc.getTableName().getNamespaceAsString());
+      String groupNameOfNs = nd.getConfigurationValue(RSGroupInfo.NAMESPACE_DESC_PROP_GROUP);
+      if (groupNameOfNs == null || groupNameOfNs.equals(RSGroupInfo.DEFAULT_GROUP)) {
+        // do not check for default group
+        return true;
+      }
+      rsGroupInfo = groupAdminServer.getRSGroupInfo(groupNameOfNs);
+      if (rsGroupInfo == null) {
+        throw new ConstraintException("RSGroup " + groupNameOfNs + " for table " +
+            desc.getTableName() + "(inherit from namespace) does not exist");
+      }
     }
+    return master.getServerManager().createDestinationServersList().stream()
+        .anyMatch(onlineServer -> rsGroupInfo.containsServer(onlineServer.getAddress()));
   }
 
   @Override
   public void preCreateTableAction(ObserverContext<MasterCoprocessorEnvironment> ctx,
-    TableDescriptor desc, RegionInfo[] regions) throws IOException {
-    if (desc.getTableName().isSystemTable()) {
-      // do not check for system tables as we may block the bootstrap.
-      return;
+      TableDescriptor desc, RegionInfo[] regions) throws IOException {
+    checkGroupExists(desc.getRegionServerGroup());
+    if (!desc.getTableName().isSystemTable() && !rsgroupHasServersOnline(desc)) {
+      throw new HBaseIOException("No online servers in the rsgroup for " + desc);
     }
-    Supplier<String> forWhom = () -> "table " + desc.getTableName();
-    RSGroupInfo rsGroupInfo = checkGroupExists(desc.getRegionServerGroup(), forWhom);
-    if (rsGroupInfo == null) {
-      // we do not set rs group info on table, check if we have one on namespace
-      String namespace = desc.getTableName().getNamespaceAsString();
-      NamespaceDescriptor nd = master.getClusterSchema().getNamespace(namespace);
-      forWhom = () -> "table " + desc.getTableName() + "(inherit from namespace)";
-      rsGroupInfo = checkGroupExists(getNamespaceGroup(nd), forWhom);
-    }
-    checkGroupNotEmpty(rsGroupInfo, forWhom);
   }
 
   @Override
   public TableDescriptor preModifyTable(ObserverContext<MasterCoprocessorEnvironment> ctx,
-    TableName tableName, TableDescriptor currentDescriptor, TableDescriptor newDescriptor)
-    throws IOException {
-    if (!currentDescriptor.getRegionServerGroup().equals(newDescriptor.getRegionServerGroup())) {
-      Supplier<String> forWhom = () -> "table " + newDescriptor.getTableName();
-      RSGroupInfo rsGroupInfo = checkGroupExists(newDescriptor.getRegionServerGroup(), forWhom);
-      checkGroupNotEmpty(rsGroupInfo, forWhom);
-    }
+      TableName tableName, TableDescriptor currentDescriptor, TableDescriptor newDescriptor)
+      throws IOException {
+    checkGroupExists(newDescriptor.getRegionServerGroup());
     return MasterObserver.super.preModifyTable(ctx, tableName, currentDescriptor, newDescriptor);
   }
 
-  private void checkNamespaceGroup(NamespaceDescriptor nd) throws IOException {
-    Supplier<String> forWhom = () -> "namespace " + nd.getName();
-    RSGroupInfo rsGroupInfo = checkGroupExists(getNamespaceGroup(nd), forWhom);
-    checkGroupNotEmpty(rsGroupInfo, forWhom);
-  }
-
   @Override
   public void preCreateNamespace(ObserverContext<MasterCoprocessorEnvironment> ctx,
-    NamespaceDescriptor ns) throws IOException {
-    checkNamespaceGroup(ns);
+      NamespaceDescriptor ns) throws IOException {
+    checkGroupExists(
+      Optional.ofNullable(ns.getConfigurationValue(RSGroupInfo.NAMESPACE_DESC_PROP_GROUP)));
   }
 
   @Override
   public void preModifyNamespace(ObserverContext<MasterCoprocessorEnvironment> ctx,
-    NamespaceDescriptor currentNsDescriptor, NamespaceDescriptor newNsDescriptor)
-    throws IOException {
-    if (!Objects.equals(
-      currentNsDescriptor.getConfigurationValue(RSGroupInfo.NAMESPACE_DESC_PROP_GROUP),
-      newNsDescriptor.getConfigurationValue(RSGroupInfo.NAMESPACE_DESC_PROP_GROUP))) {
-      checkNamespaceGroup(newNsDescriptor);
-    }
+      NamespaceDescriptor currentNsDescriptor, NamespaceDescriptor newNsDescriptor)
+      throws IOException {
+    checkGroupExists(Optional
+        .ofNullable(newNsDescriptor.getConfigurationValue(RSGroupInfo.NAMESPACE_DESC_PROP_GROUP)));
   }
 }

hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsAdmin1.java

@@ -152,14 +152,12 @@ public class TestRSGroupsAdmin1 extends TestRSGroupsBase {
     String nsName = tablePrefix + "_foo";
     String groupName = tablePrefix + "_foo";
     LOG.info("testNamespaceConstraint");
-    addGroup(groupName, 1);
+    rsGroupAdmin.addRSGroup(groupName);
     assertTrue(observer.preAddRSGroupCalled);
     assertTrue(observer.postAddRSGroupCalled);
 
     admin.createNamespace(NamespaceDescriptor.create(nsName)
       .addConfiguration(RSGroupInfo.NAMESPACE_DESC_PROP_GROUP, groupName).build());
-    RSGroupInfo rsGroupInfo = rsGroupAdmin.getRSGroupInfo(groupName);
-    rsGroupAdmin.moveServers(rsGroupInfo.getServers(), RSGroupInfo.DEFAULT_GROUP);
     // test removing a referenced group
     try {
       rsGroupAdmin.removeRSGroup(groupName);

hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/TestRSGroupsBasics.java

@@ -20,8 +20,11 @@ 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 static org.junit.Assert.fail;
 
 import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Set;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
@@ -133,6 +136,45 @@ public class TestRSGroupsBasics extends TestRSGroupsBase {
     Assert.assertEquals(1, admin.getRegions(targetServer).size());
   }
 
+  @Test
+  public void testCreateWhenRsgroupNoOnlineServers() throws Exception {
+    LOG.info("testCreateWhenRsgroupNoOnlineServers");
+
+    // set rsgroup has no online servers and test create table
+    final RSGroupInfo appInfo = addGroup("appInfo", 1);
+    Iterator<Address> iterator = appInfo.getServers().iterator();
+    List<ServerName> serversToDecommission = new ArrayList<>();
+    ServerName targetServer = getServerName(iterator.next());
+    assertTrue(master.getServerManager().getOnlineServers().containsKey(targetServer));
+    serversToDecommission.add(targetServer);
+    admin.decommissionRegionServers(serversToDecommission, true);
+    assertEquals(1, admin.listDecommissionedRegionServers().size());
+
+    final TableName tableName = TableName.valueOf(tablePrefix + "_ns", name.getMethodName());
+    admin.createNamespace(NamespaceDescriptor.create(tableName.getNamespaceAsString())
+      .addConfiguration(RSGroupInfo.NAMESPACE_DESC_PROP_GROUP, appInfo.getName()).build());
+    final TableDescriptor desc = TableDescriptorBuilder.newBuilder(tableName)
+      .setColumnFamily(ColumnFamilyDescriptorBuilder.of("f")).build();
+    try {
+      admin.createTable(desc);
+      fail("Shouldn't create table successfully!");
+    } catch (Exception e) {
+      LOG.debug("create table error", e);
+    }
+
+    // recommission and test create table
+    admin.recommissionRegionServer(targetServer, null);
+    assertEquals(0, admin.listDecommissionedRegionServers().size());
+    admin.createTable(desc);
+    // wait for created table to be assigned
+    TEST_UTIL.waitFor(WAIT_TIMEOUT, new Waiter.Predicate<Exception>() {
+      @Override
+      public boolean evaluate() throws Exception {
+        return getTableRegionMap().get(desc.getTableName()) != null;
+      }
+    });
+  }
+
   @Test
   public void testDefaultNamespaceCreateAndAssign() throws Exception {
     LOG.info("testDefaultNamespaceCreateAndAssign");