Copilot commented on code in PR #8502:
URL: https://github.com/apache/hbase/pull/8502#discussion_r3669415872
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestCacheAwareLoadBalancer.java:
##########
@@ -116,11 +123,527 @@ public static void beforeAllTests() throws Exception {
tableDescs = constructTableDesc(false);
Configuration conf = HBaseConfiguration.create();
conf.set(HConstants.BUCKET_CACHE_PERSISTENT_PATH_KEY,
"prefetch_file_list");
+ conf.setFloat(HConstants.BUCKET_CACHE_SIZE_KEY, 10);
loadBalancer = new CacheAwareLoadBalancer();
loadBalancer.setClusterInfoProvider(new DummyClusterInfoProvider(conf));
loadBalancer.loadConf(conf);
}
+ @Test
+ public void testRegionsNotCachedOnOldServerAndCurrentServer() throws
Exception {
+ // The regions are not cached on old server as well as the current server.
This causes
+ // skewness in the region allocation which should be fixed by the balancer
+
+ Map<ServerName, List<RegionInfo>> clusterState = new HashMap<>();
+ ServerName server0 = servers.get(0);
+ ServerName server1 = servers.get(1);
+ ServerName server2 = servers.get(2);
+
+ // Simulate that the regions previously hosted by server1 are now hosted
on server0
+ List<RegionInfo> regionsOnServer0 = randomRegions(10);
+ List<RegionInfo> regionsOnServer1 = randomRegions(0);
+ List<RegionInfo> regionsOnServer2 = randomRegions(5);
+
+ clusterState.put(server0, regionsOnServer0);
+ clusterState.put(server1, regionsOnServer1);
+ clusterState.put(server2, regionsOnServer2);
+
+ // Mock cluster metrics — give only server1 free cache so moves are
directed there
+ Map<ServerName, ServerMetrics> serverMetricsMap = new TreeMap<>();
+ ServerMetrics sm0 = mockServerMetricsWithRegionCacheInfo(server0,
regionsOnServer0, 0.0f,
+ new ArrayList<>(), 0, 10);
+ when(sm0.getCacheFreeSize()).thenReturn(0L);
+ ServerMetrics sm1 = mockServerMetricsWithRegionCacheInfo(server1,
regionsOnServer1, 0.0f,
+ new ArrayList<>(), 0, 10);
+ ServerMetrics sm2 = mockServerMetricsWithRegionCacheInfo(server2,
regionsOnServer2, 0.0f,
+ new ArrayList<>(), 0, 10);
+ when(sm2.getCacheFreeSize()).thenReturn(0L);
+ serverMetricsMap.put(server0, sm0);
+ serverMetricsMap.put(server1, sm1);
+ serverMetricsMap.put(server2, sm2);
+ ClusterMetrics clusterMetrics = mock(ClusterMetrics.class);
+ when(clusterMetrics.getLiveServerMetrics()).thenReturn(serverMetricsMap);
+ loadBalancer.updateClusterMetrics(clusterMetrics);
+
+ Map<TableName, Map<ServerName, List<RegionInfo>>> LoadOfAllTable =
+ (Map) mockClusterServersWithTables(clusterState);
+ List<RegionPlan> plans = loadBalancer.balanceCluster(LoadOfAllTable);
+ Set<RegionInfo> regionsMovedFromServer0 = new HashSet<>();
+ Map<ServerName, List<RegionInfo>> targetServers = new HashMap<>();
+ for (RegionPlan plan : plans) {
+ if (plan.getSource().equals(server0)) {
+ regionsMovedFromServer0.add(plan.getRegionInfo());
+ if (!targetServers.containsKey(plan.getDestination())) {
+ targetServers.put(plan.getDestination(), new ArrayList<>());
+ }
+ targetServers.get(plan.getDestination()).add(plan.getRegionInfo());
+ }
+ }
+ // should move at least 5 regions from server0 to balance cluster (10/0/5
-> ~5/5/5)
+ assertTrue(regionsMovedFromServer0.size() >= 5,
+ "Expected at least 5 moves from server0, got " +
regionsMovedFromServer0.size());
+ }
+
+ /**
+ * Regions on the overloaded RS report low block-cache ratio; no RS reports
prefetch/historical
+ * cache for those regions (so {@link
CacheAwareLoadBalancer.CacheAwareCandidateGenerator} has no
+ * "old server" to prefer). Another RS has ample free block cache. The
balancer should still emit
+ * plans that shed load from the hot RS onto the idle RS with spare cache
capacity.
+ */
+ @Test
+ public void
testLowCacheRatioNoHistoricalCacheRelocatesWhenTargetHasFreeBlockCache()
+ throws Exception {
+ Map<ServerName, List<RegionInfo>> clusterState = new HashMap<>();
+ ServerName server0 = servers.get(0);
+ ServerName server1 = servers.get(1);
+ ServerName server2 = servers.get(2);
+
+ List<RegionInfo> regionsOnServer0 = randomRegions(10);
+ List<RegionInfo> regionsOnServer1 = randomRegions(0);
+ List<RegionInfo> regionsOnServer2 = randomRegions(5);
+
+ clusterState.put(server0, regionsOnServer0);
+ clusterState.put(server1, regionsOnServer1);
+ clusterState.put(server2, regionsOnServer2);
+
+ // Below LOW_CACHE_RATIO_FOR_RELOCATION_DEFAULT (0.35);
+ ServerMetrics sm0 = mockServerMetricsWithRegionCacheInfo(server0,
regionsOnServer0, 0.1f,
+ new ArrayList<>(), 0, 10);
+ when(sm0.getCacheFreeSize()).thenReturn(0L);
+ ServerMetrics sm1 = mockServerMetricsWithRegionCacheInfo(server1,
regionsOnServer1, 0.0f,
+ new ArrayList<>(), 0, 10);
+ // Simulates 1GB free cache space on server1
+ when(sm1.getCacheFreeSize()).thenReturn(1024L * 1024 * 1024);
+ ServerMetrics sm2 = mockServerMetricsWithRegionCacheInfo(server2,
regionsOnServer2, 1.0f,
+ new ArrayList<>(), 0, 10);
+ when(sm2.getCacheFreeSize()).thenReturn(0L);
+
+ Map<ServerName, ServerMetrics> serverMetricsMap = new TreeMap<>();
+ serverMetricsMap.put(server0, sm0);
+ serverMetricsMap.put(server1, sm1);
+ serverMetricsMap.put(server2, sm2);
+ ClusterMetrics clusterMetrics = mock(ClusterMetrics.class);
+ when(clusterMetrics.getLiveServerMetrics()).thenReturn(serverMetricsMap);
+ loadBalancer.updateClusterMetrics(clusterMetrics);
+
+ assertTrue(loadBalancer.regionCacheRatioOnOldServerMap.isEmpty());
+
+ Map<TableName, Map<ServerName, List<RegionInfo>>> loadOfAllTable =
+ (Map) mockClusterServersWithTables(clusterState);
+ List<RegionPlan> plans = loadBalancer.balanceCluster(loadOfAllTable);
+ assertNotNull(plans);
+
+ Set<RegionInfo> regionsMovedFromServer0 = new HashSet<>();
+ Map<ServerName, List<RegionInfo>> targetServers = new HashMap<>();
+ for (RegionPlan plan : plans) {
+ if (plan.getSource().equals(server0)) {
+ regionsMovedFromServer0.add(plan.getRegionInfo());
+ if (!targetServers.containsKey(plan.getDestination())) {
+ targetServers.put(plan.getDestination(), new ArrayList<>());
+ }
+ targetServers.get(plan.getDestination()).add(plan.getRegionInfo());
+ }
+ }
+ assertEquals(5, regionsMovedFromServer0.size());
+ assertNotNull(targetServers.get(server1));
+ assertEquals(5, targetServers.get(server1).size());
+ }
+
+ @Test
+ public void
testRegionsPartiallyCachedOnOldServerAndNotCachedOnCurrentServer() throws
Exception {
+ // The regions are partially cached on old server but not cached on the
current server
+
+ Map<ServerName, List<RegionInfo>> clusterState = new HashMap<>();
+ ServerName server0 = servers.get(0);
+ ServerName server1 = servers.get(1);
+ ServerName server2 = servers.get(2);
+
+ // Simulate that the regions previously hosted by server1 are now hosted
on server0
+ List<RegionInfo> regionsOnServer0 = randomRegions(10);
+ List<RegionInfo> regionsOnServer1 = randomRegions(0);
+ List<RegionInfo> regionsOnServer2 = randomRegions(5);
+
+ clusterState.put(server0, regionsOnServer0);
+ clusterState.put(server1, regionsOnServer1);
+ clusterState.put(server2, regionsOnServer2);
+
+ // Mock cluster metrics
+
+ // Mock 5 regions from server0 were previously hosted on server1
Review Comment:
The comment says "Mock 5 regions" but the subList call on the next line (end
index = size() - 1) produces 4 regions. This is misleading when reading the
test setup.
This issue also appears on line 485 of the same file.
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/CacheAwareLoadBalancer.java:
##########
@@ -383,6 +431,31 @@ private boolean moveRegionToOldServer(BalancerClusterState
cluster, int regionIn
return false;
}
+ // If the region is already well-cached on its current server, don't
disrupt it.
+ // The old server's historical cache data may be stale, and moving a hot
region
+ // causes unnecessary cache churn.
+ if (cacheRatioOnCurrentServer >= ratioThreshold) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "Region {} not moved from {} to {} as it is already well-cached
({}) on current server",
+ cluster.regions[regionIndex].getEncodedName(),
cluster.servers[currentServerIndex],
+ cluster.servers[oldServerIndex], cacheRatioOnCurrentServer);
+ }
+ return false;
+ }
+
+ if (!serverHasCacheSpaceForRegion(cluster, regionIndex, oldServerIndex))
{
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Region {} not moved from {} to {} as destination server
lacks cache space",
+ cluster.regions[regionIndex].getEncodedName(),
cluster.servers[currentServerIndex],
+ cluster.servers[oldServerIndex]);
+ }
+ return false;
+ }
+
+ DecimalFormat df = new DecimalFormat("#");
+ df.setMaximumFractionDigits(4);
Review Comment:
DecimalFormat is created with pattern "#", which suppresses the decimal
separator. Even with setMaximumFractionDigits(4), this can round 0.8 to "1" in
the debug log, making the cache ratio output misleading.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
