Skip to content

HBASE-29072 StochasticLoadBalancer#areReplicasColocated ignores rack colocation #6622

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rmdmattingly merged 1 commit into from
Jan 27, 2025
Merged

HBASE-29072 StochasticLoadBalancer#areReplicasColocated ignores rack colocation #6622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ...-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BalancerClusterState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ class BalancerClusterState {
int numRacks;
int numTables;
int numRegions;
int maxReplicas = 1;

int numMovedRegions = 0; // num moved regions from the initial configuration
Map<ServerName, List<RegionInfo>> clusterState;
Expand Down Expand Up @@ -446,6 +447,11 @@ private void registerRegion(RegionInfo region, int regionIndex, int serverIndex,
: serversToIndex.get(loc.get(i).getAddress()));
}
}

int numReplicas = region.getReplicaId() + 1;
if (numReplicas > maxReplicas) {
maxReplicas = numReplicas;
}
Comment on lines +451 to +454
Copy link
Contributor Author

@rmdmattingly rmdmattingly Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seemed like a very cheap way to determine the max replica count without actually seeing the table descriptors, which is a limitation of the current balancer implementation.

Sometime I would like to fix this limitation and make the balancer table descriptor aware, but I think that is its own problem

Copy link
Member

@ndimiduk ndimiduk Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure that you have insufficient information here. I'm looking at the call hierarchy of registerRegion and I believe that there's no limit by table. So you're approximating the maximum number of region replicas used by any table in the cluster? I suspect that you need to introduce a new index by table name to the number of replicas per table.

Copy link
Contributor Author

@rmdmattingly rmdmattingly Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, we could make this better with table specific replica count tracking

Copy link
Member

@ndimiduk ndimiduk Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay but that additional tracking is not a correctness issue for how this value is used?

Copy link
Contributor Author

@rmdmattingly rmdmattingly Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The BalancerClusterState is only aware of the regions for tables that it is actively trying to balance — so if you're balancing by table (not the default) then I believe table specific tracking will not make this any better. If you're balancing by cluster (the default) then I believe table specific tracking could maybe make things better by checking for rack replica colocation by table... actually now that I think about it the cost functions probably don't support this. So, tldr, if you're doing cluster-wide balancing then this will cause us to ignore rack colocation as a trigger once any single table's replica counts are greater than the number of racks, and the way to get more specific decisions would be to enable byTable balancing

}

/**
Expand Down
40 changes: 35 additions & 5 deletions ...alancer/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,10 +331,33 @@ void updateMetricsSize(int size) {
}
}

private boolean areSomeRegionReplicasColocated(BalancerClusterState c) {
regionReplicaHostCostFunction.prepare(c);
double cost = Math.abs(regionReplicaHostCostFunction.cost());
return cost > CostFunction.getCostEpsilon(cost);
private boolean areSomeRegionReplicasColocatedOnHost(BalancerClusterState c) {
if (c.numHosts >= c.maxReplicas) {
regionReplicaHostCostFunction.prepare(c);
double hostCost = Math.abs(regionReplicaHostCostFunction.cost());
boolean colocatedAtHost = hostCost > CostFunction.getCostEpsilon(hostCost);
if (colocatedAtHost) {
return true;
}
LOG.trace("No host colocation detected with host cost={}", hostCost);
}
return false;
}

private boolean areSomeRegionReplicasColocatedOnRack(BalancerClusterState c) {
if (c.numRacks >= c.maxReplicas) {
regionReplicaRackCostFunction.prepare(c);
double rackCost = Math.abs(regionReplicaRackCostFunction.cost());
boolean colocatedAtRack = rackCost > CostFunction.getCostEpsilon(rackCost);
if (colocatedAtRack) {
return true;
}
LOG.trace("No rack colocation detected with rack cost={}", rackCost);
} else {
LOG.trace("Rack colocation is inevitable with fewer racks than replicas, "
+ "so we won't bother checking");
}
return false;
}

private String getBalanceReason(double total, double sumMultiplier) {
Expand All @@ -361,12 +384,19 @@ boolean needsBalance(TableName tableName, BalancerClusterState cluster) {
+ " < MIN_SERVER_BALANCE(" + MIN_SERVER_BALANCE + ")", null);
return false;
}
if (areSomeRegionReplicasColocated(cluster)) {

if (areSomeRegionReplicasColocatedOnHost(cluster)) {
LOG.info("Running balancer because at least one server hosts replicas of the same region."
+ " function cost={}", functionCost());
return true;
}

if (areSomeRegionReplicasColocatedOnRack(cluster)) {
LOG.info("Running balancer because at least one rack hosts replicas of the same region."
+ " function cost={}", functionCost());
return true;
}

if (idleRegionServerExist(cluster)) {
LOG.info("Running balancer because cluster has idle server(s)." + " function cost={}",
functionCost());
Expand Down
50 changes: 41 additions & 9 deletions ...java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancerRegionReplica.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@
import org.junit.Test;
import org.junit.experimental.categories.Category;

import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableList;

@Category({ MasterTests.class, LargeTests.class })
public class TestStochasticLoadBalancerRegionReplica extends StochasticBalancerTestBase {

Expand Down Expand Up @@ -136,7 +138,7 @@ public void testReplicaCostForReplicas() {
}

@Test
public void testNeedsBalanceForColocatedReplicas() {
public void testNeedsBalanceForColocatedReplicasOnHost() {
// check for the case where there are two hosts and with one rack, and where
// both the replicas are hosted on the same server
List<RegionInfo> regions = randomRegions(1);
Expand All @@ -148,20 +150,50 @@ public void testNeedsBalanceForColocatedReplicas() {
// until the step above s1 holds two replicas of a region
regions = randomRegions(1);
map.put(s2, regions);
assertTrue(loadBalancer.needsBalance(HConstants.ENSEMBLE_TABLE_NAME,
new BalancerClusterState(map, null, null, null)));
// check for the case where there are two hosts on the same rack and there are two racks
// and both the replicas are on the same rack
map.clear();
regions = randomRegions(1);
BalancerClusterState cluster =
new BalancerClusterState(map, null, null, new ForTestRackManagerOne());
loadBalancer.initCosts(cluster);
assertTrue(loadBalancer.needsBalance(HConstants.ENSEMBLE_TABLE_NAME, cluster));
}

@Test
public void testNeedsBalanceForColocatedReplicasOnRack() {
// Three hosts, two racks, and two replicas for a region. This should be balanced
List<RegionInfo> regions = randomRegions(1);
ServerName s1 = ServerName.valueOf("host1", 1000, 11111);
ServerName s2 = ServerName.valueOf("host11", 1000, 11111);
Map<ServerName, List<RegionInfo>> map = new HashMap<>();
List<RegionInfo> regionsOnS2 = new ArrayList<>(1);
regionsOnS2.add(RegionReplicaUtil.getRegionInfoForReplica(regions.get(0), 1));
map.put(s1, regions);
map.put(s2, regionsOnS2);
// add another server so that the cluster has some host on another rack
map.put(ServerName.valueOf("host2", 1000, 11111), randomRegions(1));
assertFalse(loadBalancer.needsBalance(HConstants.ENSEMBLE_TABLE_NAME,
new BalancerClusterState(map, null, null, new ForTestRackManagerOne())));
Comment on lines -163 to -164
Copy link
Contributor Author

@rmdmattingly rmdmattingly Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that PR where we made the balancer ignorant to rack colocation, we also just naively switched this assertion to false. But the intention of this test was always for the assertion to be true.

I've also split this test up into two tests, because it was always testing two distinct cases (host and rack colocation).

BalancerClusterState cluster =
new BalancerClusterState(map, null, null, new ForTestRackManagerOne());
loadBalancer.initCosts(cluster);
assertTrue(loadBalancer.needsBalance(HConstants.ENSEMBLE_TABLE_NAME, cluster));
}

@Test
public void testNoNeededBalanceForColocatedReplicasTooFewRacks() {
// Three hosts, two racks, and three replicas for a region. This cannot be balanced
List<RegionInfo> regions = randomRegions(1);
ServerName s1 = ServerName.valueOf("host1", 1000, 11111);
ServerName s2 = ServerName.valueOf("host11", 1000, 11111);
ServerName s3 = ServerName.valueOf("host2", 1000, 11111);
Map<ServerName, List<RegionInfo>> map = new HashMap<>();
List<RegionInfo> regionsOnS2 = new ArrayList<>(1);
regionsOnS2.add(RegionReplicaUtil.getRegionInfoForReplica(regions.get(0), 1));
map.put(s1, regions);
map.put(s2, regionsOnS2);
// there are 3 replicas for region 0, but only add a second rack
map.put(s3, ImmutableList.of(RegionReplicaUtil.getRegionInfoForReplica(regions.get(0), 2)));
BalancerClusterState cluster =
new BalancerClusterState(map, null, null, new ForTestRackManagerOne());
loadBalancer.initCosts(cluster);
// Should be false because there aren't enough racks
assertFalse(loadBalancer.needsBalance(HConstants.ENSEMBLE_TABLE_NAME, cluster));
}

@Test
Expand Down