HBASE-30331 Remove RegionMover.filterRSGroupServers's short-circuit on default RSGroup

hbase-server/src/main/java/org/apache/hadoop/hbase/util/RegionMover.java

@@ -527,9 +527,6 @@ private boolean unloadRegions(boolean unloadFromRack, List<String> isolateRegion
   @InterfaceAudience.Private
   Collection<ServerName> filterRSGroupServers(RSGroupInfo rsgroup,
     Collection<ServerName> onlineServers) {
-    if (rsgroup.getName().equals(RSGroupInfo.DEFAULT_GROUP)) {
-      return onlineServers;
-    }
     List<ServerName> serverLists = new ArrayList<>(rsgroup.getServers().size());
     for (ServerName server : onlineServers) {
       Address address = Address.fromParts(server.getHostname(), server.getPort());

hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestRegionMoverFilterRSGroupServers.java

@@ -0,0 +1,204 @@
+/*
+ * 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.util;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import org.apache.hadoop.hbase.HBaseTestingUtil;
+import org.apache.hadoop.hbase.ServerName;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.net.Address;
+import org.apache.hadoop.hbase.rsgroup.RSGroupInfo;
+import org.apache.hadoop.hbase.rsgroup.RSGroupUtil;
+import org.apache.hadoop.hbase.testclassification.MediumTests;
+import org.apache.hadoop.hbase.testclassification.MiscTests;
+import org.apache.hadoop.hbase.util.RegionMover.RegionMoverBuilder;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Unit tests for {@link RegionMover#filterRSGroupServers}. RSGroups is enabled on the mini cluster
+ * and servers are moved between groups via the real {@code moveServersToRSGroup} admin call -- no
+ * test constructs an {@link RSGroupInfo} by hand. Every {@code RSGroupInfo} used here is read back
+ * from the master via {@code admin.getRSGroup(...)}, which computes "default" membership as "online
+ * servers minus servers claimed by other groups" (see RSGroupInfoManagerImpl#getDefaultServers), so
+ * its membership shrinks on its own once a move happens instead of being asserted into existence by
+ * the test.
+ */
+@Tag(MiscTests.TAG)
+@Tag(MediumTests.TAG)
+public class TestRegionMoverFilterRSGroupServers {
+
+  private static final HBaseTestingUtil TEST_UTIL = new HBaseTestingUtil();
+
+  @BeforeAll
+  public static void setUpBeforeClass() throws Exception {
+    RSGroupUtil.enableRSGroup(TEST_UTIL.getConfiguration());
+    TEST_UTIL.startMiniCluster(2);
+  }
+
+  @AfterAll
+  public static void tearDownAfterClass() throws Exception {
+    TEST_UTIL.shutdownMiniCluster();
+  }
+
+  private RegionMover buildMover() throws Exception {
+    ServerName any = TEST_UTIL.getAdmin().getRegionServers().iterator().next();
+    return new RegionMoverBuilder(any.getHostname() + ":" + any.getPort(),
+      TEST_UTIL.getConfiguration()).build();
+  }
+
+  private static Address addressOf(ServerName sn) {
+    return Address.fromParts(sn.getHostname(), sn.getPort());
+  }
+
+  /**
+   * Reproduces HBASE-30331: once a server is moved out of "default" into a custom group, the
+   * master-computed "default" RSGroupInfo only lists the server that remains. filterRSGroupServers
+   * must honor that real membership instead of short-circuiting on the group name and returning
+   * every online server.
+   */
+  @Test
+  public void testDefaultGroupFiltersToActualMembers() throws Exception {
+    Admin admin = TEST_UTIL.getAdmin();
+    List<ServerName> allServers = new ArrayList<>(admin.getRegionServers());
+    assertEquals(2, allServers.size(), "Mini cluster should have started with 2 region servers");
+
+    ServerName movedOut = allServers.get(0);
+    ServerName inDefault = allServers.get(1);
+    String groupName = "test_default_filter";
+
+    admin.addRSGroup(groupName);
+    admin.moveServersToRSGroup(new HashSet<>(List.of(addressOf(movedOut))), groupName);
+    try {
+      // Master-computed membership, not something we constructed ourselves.
+      RSGroupInfo defaultGroup = admin.getRSGroup(RSGroupInfo.DEFAULT_GROUP);
+      assertEquals(1, defaultGroup.getServers().size(),
+        "Master-computed default group should shrink to 1 member after moving the other "
+          + "server out");
+      assertTrue(defaultGroup.containsServer(addressOf(inDefault)),
+        "Server that was not moved out must remain in the default group's real membership");
+
+      try (RegionMover rm = buildMover()) {
+        Collection<ServerName> result = rm.filterRSGroupServers(defaultGroup, allServers);
+
+        assertEquals(1, result.size(),
+          "filterRSGroupServers should return only the default group's actual members");
+        assertTrue(result.contains(inDefault),
+          "Server that is an actual member of the default group must be returned as a "
+            + "destination");
+        assertFalse(result.contains(movedOut),
+          "Server moved out of default must not be returned as a destination just because the "
+            + "group being filtered is named 'default'");
+      }
+    } finally {
+      admin.moveServersToRSGroup(new HashSet<>(List.of(addressOf(movedOut))),
+        RSGroupInfo.DEFAULT_GROUP);
+      admin.removeRSGroup(groupName);
+    }
+  }
+
+  /**
+   * A non-default group with one member must return only that member. Same real-cluster approach as
+   * above: the group and its membership come from actual {@code moveServersToRSGroup} calls, not a
+   * hand-built {@link RSGroupInfo}.
+   */
+  @Test
+  public void testNonDefaultGroupFiltersToMembers() throws Exception {
+    Admin admin = TEST_UTIL.getAdmin();
+    List<ServerName> allServers = new ArrayList<>(admin.getRegionServers());
+    assertEquals(2, allServers.size(), "Mini cluster should have started with 2 region servers");
+
+    ServerName member = allServers.get(0);
+    ServerName other = allServers.get(1);
+    String groupName = "test_nondefault_filter";
+
+    admin.addRSGroup(groupName);
+    admin.moveServersToRSGroup(new HashSet<>(List.of(addressOf(member))), groupName);
+    try {
+      RSGroupInfo group = admin.getRSGroup(groupName);
+      assertEquals(1, group.getServers().size(),
+        "Master-computed group should have exactly the one server moved into it");
+      assertTrue(group.containsServer(addressOf(member)),
+        "Server moved into the group must be part of its real membership");
+
+      try (RegionMover rm = buildMover()) {
+        Collection<ServerName> result = rm.filterRSGroupServers(group, allServers);
+        assertEquals(1, result.size(),
+          "filterRSGroupServers should return only the non-default group's actual members");
+        assertTrue(result.contains(member),
+          "Server that is an actual member of the group must be returned as a destination");
+        assertFalse(result.contains(other),
+          "Server that is not a member of the group must not be returned as a destination");
+      }
+    } finally {
+      admin.moveServersToRSGroup(new HashSet<>(List.of(addressOf(member))),
+        RSGroupInfo.DEFAULT_GROUP);
+      admin.removeRSGroup(groupName);
+    }
+  }
+
+  /**
+   * A group's real member must not be returned as a destination when it is absent from the
+   * {@code onlineServers} snapshot handed to the filter (e.g. the server is currently offline or
+   * was already excluded upstream). Uses a real group/member from {@code moveServersToRSGroup}, not
+   * a fabricated, never-existed host address.
+   */
+  @Test
+  public void testGroupMemberAbsentFromOnlineServersReturnsEmpty() throws Exception {
+    Admin admin = TEST_UTIL.getAdmin();
+    List<ServerName> allServers = new ArrayList<>(admin.getRegionServers());
+    assertEquals(2, allServers.size(), "Mini cluster should have started with 2 region servers");
+
+    ServerName member = allServers.get(0);
+    ServerName other = allServers.get(1);
+    String groupName = "test_absent_filter";
+
+    admin.addRSGroup(groupName);
+    admin.moveServersToRSGroup(new HashSet<>(List.of(addressOf(member))), groupName);
+    try {
+      RSGroupInfo group = admin.getRSGroup(groupName);
+      assertEquals(1, group.getServers().size(),
+        "Master-computed group should have exactly the one server moved into it");
+      assertTrue(group.containsServer(addressOf(member)),
+        "Server moved into the group must be part of its real membership");
+
+      try (RegionMover rm = buildMover()) {
+        // The group's only member is not part of the online-servers snapshot passed in.
+        Collection<ServerName> result =
+          rm.filterRSGroupServers(group, Collections.singletonList(other));
+        assertTrue(result.isEmpty(),
+          "Group member absent from the online-servers snapshot must not be returned as a "
+            + "destination");
+      }
+    } finally {
+      admin.moveServersToRSGroup(new HashSet<>(List.of(addressOf(member))),
+        RSGroupInfo.DEFAULT_GROUP);
+      admin.removeRSGroup(groupName);
+    }
+  }
+}

hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestRegionMoverWithRSGroupEnable.java

@@ -18,6 +18,8 @@
 package org.apache.hadoop.hbase.util;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.util.ArrayList;
 import java.util.Collection;
@@ -32,13 +34,15 @@
 import org.apache.hadoop.hbase.client.TableDescriptor;
 import org.apache.hadoop.hbase.client.TableDescriptorBuilder;
 import org.apache.hadoop.hbase.net.Address;
+import org.apache.hadoop.hbase.regionserver.HRegion;
 import org.apache.hadoop.hbase.regionserver.HRegionServer;
 import org.apache.hadoop.hbase.rsgroup.RSGroupInfo;
 import org.apache.hadoop.hbase.rsgroup.RSGroupUtil;
 import org.apache.hadoop.hbase.testclassification.MediumTests;
 import org.apache.hadoop.hbase.testclassification.MiscTests;
 import org.apache.hadoop.hbase.util.RegionMover.RegionMoverBuilder;
 import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Tag;
@@ -69,7 +73,11 @@ public static void tearDownAfterClass() throws Exception {
     TEST_UTIL.shutdownMiniCluster();
   }
 
+  private static final TableName TABLE_NAME = TableName.valueOf("testRegionMoverWithRSGroupEnable");
+
   private final List<Address> rsservers = new ArrayList<>(2);
+  private final List<ServerName> defaultGroupServers = new ArrayList<>();
+  private ServerName rsContainMeta;
 
   @BeforeEach
   public void setUp() throws Exception {
@@ -79,7 +87,7 @@ public void setUp() throws Exception {
     admin.addRSGroup(TEST_RSGROUP);
     Collection<ServerName> allServers = admin.getRegionServers();
     // Remove rs contains hbase:meta, otherwise test looks unstable and buggy in test env.
-    ServerName rsContainMeta = TEST_UTIL.getMiniHBaseCluster().getRegionServerThreads().stream()
+    rsContainMeta = TEST_UTIL.getMiniHBaseCluster().getRegionServerThreads().stream()
       .map(t -> t.getRegionServer())
       .filter(rs -> rs.getRegions(TableName.META_TABLE_NAME).size() > 0).findFirst().get()
       .getServerName();
@@ -97,19 +105,42 @@ public void setUp() throws Exception {
     assertEquals(3, admin.getRSGroup(RSGroupInfo.DEFAULT_GROUP).getServers().size());
     assertEquals(2, admin.getRSGroup(TEST_RSGROUP).getServers().size());
 
+    // Track the servers left in the default group, used for isolation assertions.
+    for (ServerName server : allServers) {
+      if (!rsservers.contains(Address.fromParts(server.getHostname(), server.getPort()))) {
+        defaultGroupServers.add(server);
+      }
+    }
+
     // Create a pre-split table in test rsgroup
-    TableName tableName = TableName.valueOf("testRegionMoverWithRSGroupEnable");
-    if (admin.tableExists(tableName)) {
-      TEST_UTIL.deleteTable(tableName);
+    if (admin.tableExists(TABLE_NAME)) {
+      TEST_UTIL.deleteTable(TABLE_NAME);
     }
-    TableDescriptor tableDesc = TableDescriptorBuilder.newBuilder(tableName)
+    TableDescriptor tableDesc = TableDescriptorBuilder.newBuilder(TABLE_NAME)
       .setColumnFamily(ColumnFamilyDescriptorBuilder.of("f")).setRegionServerGroup(TEST_RSGROUP)
       .build();
     String startKey = "a";
     String endKey = "z";
     admin.createTable(tableDesc, Bytes.toBytes(startKey), Bytes.toBytes(endKey), 9);
   }
 
+  @AfterEach
+  public void tearDown() throws Exception {
+    Admin admin = TEST_UTIL.getAdmin();
+    if (admin.tableExists(TABLE_NAME)) {
+      TEST_UTIL.deleteTable(TABLE_NAME);
+    }
+    if (!rsservers.isEmpty()) {
+      admin.moveServersToRSGroup(new HashSet<>(rsservers), RSGroupInfo.DEFAULT_GROUP);
+    }
+    if (admin.getRSGroup(TEST_RSGROUP) != null) {
+      admin.removeRSGroup(TEST_RSGROUP);
+    }
+    rsservers.clear();
+    defaultGroupServers.clear();
+    rsContainMeta = null;
+  }
+
   @Test
   public void testUnloadRegions() throws Exception {
     Address decommission = rsservers.get(0);
@@ -125,6 +156,82 @@ public void testUnloadRegions() throws Exception {
       .map(JVMClusterUtil.RegionServerThread::getRegionServer)
       .filter(rs -> rs.getServerName().getAddress().equals(online)).findFirst().get();
     assertEquals(9, onlineRS.getNumberOfOnlineRegions());
+
+    // Isolation assertion: no default-group server must hold any region of the test table.
+    for (ServerName defaultSN : defaultGroupServers) {
+      HRegionServer defaultRS = TEST_UTIL.getMiniHBaseCluster().getRegionServerThreads().stream()
+        .map(JVMClusterUtil.RegionServerThread::getRegionServer)
+        .filter(rs -> rs.getServerName().equals(defaultSN)).findFirst().orElse(null);
+      if (defaultRS == null) {
+        continue;
+      }
+      List<HRegion> tableRegions = defaultRS.getRegions(TABLE_NAME);
+      assertTrue(tableRegions.isEmpty(), "Default-group server " + defaultSN
+        + " must not hold any regions of " + TABLE_NAME + " but had: " + tableRegions);
+    }
+  }
+
+  /**
+   * Unloading a server that is in the default RSGroup must still succeed end-to-end when RSGroups
+   * are enabled. Destinations must be filtered to the default group: regions may spread across the
+   * other default-group servers, but must not land on any test-group server.
+   */
+  @Test
+  public void testUnloadDefaultGroupServerWithRSGroupEnabled() throws Exception {
+    Admin admin = TEST_UTIL.getAdmin();
+    // Avoid unloading the meta-carrying server here too, for the same stability reason setUp()
+    // avoids it when picking rsservers.
+    ServerName defaultSN =
+      defaultGroupServers.stream().filter(sn -> !sn.equals(rsContainMeta)).findFirst().get();
+    Address decommission = defaultSN.getAddress();
+    String filename = new Path(TEST_UTIL.getDataTestDir(), "testDefaultGroupUnload").toString();
+
+    // Create a table in the default group; the balancer will distribute its regions naturally
+    // across the default-group servers, so defaultSN will hold at least some.
+    TableName defaultTable = TableName.valueOf("testDefaultGroupTable");
+    if (admin.tableExists(defaultTable)) {
+      TEST_UTIL.deleteTable(defaultTable);
+    }
+    try {
+      TableDescriptor td = TableDescriptorBuilder.newBuilder(defaultTable)
+        .setColumnFamily(ColumnFamilyDescriptorBuilder.of("f")).build();
+      admin.createTable(td, Bytes.toBytes("a"), Bytes.toBytes("z"), 6);
+      TEST_UTIL.waitTableAvailable(defaultTable);
+
+      HRegionServer decommRS = TEST_UTIL.getMiniHBaseCluster().getRegionServerThreads().stream()
+        .map(JVMClusterUtil.RegionServerThread::getRegionServer)
+        .filter(rs -> rs.getServerName().equals(defaultSN)).findFirst().get();
+      assertFalse(decommRS.getRegions(defaultTable).isEmpty(),
+        "Precondition: decommissioned server must actually host some regions of the default "
+          + "table, otherwise the post-unload check below is vacuous");
+
+      RegionMoverBuilder builder =
+        new RegionMoverBuilder(decommission.toString(), TEST_UTIL.getConfiguration());
+      try (RegionMover rm = builder.filename(filename).ack(true).build()) {
+        LOG.info("Unloading default-group server {}", decommission.getHostname());
+        rm.unload();
+      }
+
+      // After unload, the decommissioned server must hold no regions of the default table.
+      assertEquals(0, decommRS.getRegions(defaultTable).size(),
+        "Decommissioned default-group server must hold no regions after unload");
+
+      // Isolation assertion: no test-group server must hold any region of the default table.
+      for (JVMClusterUtil.RegionServerThread rst : TEST_UTIL.getMiniHBaseCluster()
+        .getRegionServerThreads()) {
+        HRegionServer rs = rst.getRegionServer();
+        Address addr = rs.getServerName().getAddress();
+        if (rsservers.contains(addr)) {
+          List<HRegion> found = rs.getRegions(defaultTable);
+          assertTrue(found.isEmpty(), "Test-group server " + addr + " must not hold any region of "
+            + defaultTable + " but had: " + found);
+        }
+      }
+    } finally {
+      if (admin.tableExists(defaultTable)) {
+        TEST_UTIL.deleteTable(defaultTable);
+      }
+    }
   }
 
 }