This is an automated email from the ASF dual-hosted git repository.
virajjasani pushed a commit to branch branch-3.0
in repository https://gitbox.apache.org/repos/asf/hbase.git
The following commit(s) were added to refs/heads/branch-3.0 by this push:
new 5383822eb33 HBASE-30331 Remove RegionMover.filterRSGroupServers's
short-circuit on default RSGroup (#8552)
5383822eb33 is described below
commit 5383822eb3355355c8ad51b9d5b52279ef055b4f
Author: Umesh <[email protected]>
AuthorDate: Thu Aug 20 09:59:36 2026 +0530
HBASE-30331 Remove RegionMover.filterRSGroupServers's short-circuit on
default RSGroup (#8552)
Co-authored-by: Claude Sonnet 5 <[email protected]>
Signed-off-by: Viraj Jasani <[email protected]>
---
.../org/apache/hadoop/hbase/util/RegionMover.java | 3 -
.../util/TestRegionMoverFilterRSGroupServers.java | 204 +++++++++++++++++++++
.../util/TestRegionMoverWithRSGroupEnable.java | 117 +++++++++++-
3 files changed, 316 insertions(+), 8 deletions(-)
diff --git
a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/RegionMover.java
b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/RegionMover.java
index 8b127a02beb..003ce583518 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/RegionMover.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/RegionMover.java
@@ -526,9 +526,6 @@ public class RegionMover extends AbstractHBaseTool
implements Closeable {
@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());
diff --git
a/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestRegionMoverFilterRSGroupServers.java
b/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestRegionMoverFilterRSGroupServers.java
new file mode 100644
index 00000000000..66838282d64
--- /dev/null
+++
b/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);
+ }
+ }
+}
diff --git
a/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestRegionMoverWithRSGroupEnable.java
b/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestRegionMoverWithRSGroupEnable.java
index 3d261da9278..4a8dad55f7b 100644
---
a/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestRegionMoverWithRSGroupEnable.java
+++
b/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,6 +34,7 @@ import
org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder;
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;
@@ -39,6 +42,7 @@ 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 class TestRegionMoverWithRSGroupEnable {
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 class TestRegionMoverWithRSGroupEnable {
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,12 +105,18 @@ public class TestRegionMoverWithRSGroupEnable {
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";
@@ -110,6 +124,23 @@ public class TestRegionMoverWithRSGroupEnable {
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 class TestRegionMoverWithRSGroupEnable {
.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);
+ }
+ }
}
}
