Skip to content

Commit c755e7b

Browse files
authored
Speedup BalanceUnbalancedClusterTests (#88794)
This commit speeds up the above tests (and probably many others) by changing how we assert the invariant. Previously invariant was checked by rebuilding internal collections from scratch and comparing them against ones already present in the object after every single modification twice. This commit verifies the invariant once after all bulk changes.
1 parent 664c12c commit c755e7b

File tree

2 files changed

+29
-20
lines changed

2 files changed

+29
-20
lines changed

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNode.java

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
import org.elasticsearch.index.shard.ShardId;
1616

1717
import java.util.ArrayList;
18-
import java.util.Collection;
1918
import java.util.Collections;
2019
import java.util.HashSet;
2120
import java.util.Iterator;
@@ -26,7 +25,6 @@
2625
import java.util.Objects;
2726
import java.util.Set;
2827
import java.util.function.Predicate;
29-
import java.util.stream.Collectors;
3028

3129
/**
3230
* A {@link RoutingNode} represents a cluster node associated with a single {@link DiscoveryNode} including all shards
@@ -118,7 +116,14 @@ public int size() {
118116
* @param shard Shard to create on this Node
119117
*/
120118
void add(ShardRouting shard) {
121-
assert invariant();
119+
addInternal(shard, true);
120+
}
121+
122+
void addWithoutValidation(ShardRouting shard) {
123+
addInternal(shard, false);
124+
}
125+
126+
private void addInternal(ShardRouting shard, boolean validate) {
122127
final ShardRouting existing = shards.putIfAbsent(shard.shardId(), shard);
123128
if (existing != null) {
124129
final IllegalStateException e = new IllegalStateException(
@@ -142,11 +147,10 @@ void add(ShardRouting shard) {
142147
relocatingShards.add(shard);
143148
}
144149
shardsByIndex.computeIfAbsent(shard.index(), k -> new HashSet<>()).add(shard);
145-
assert invariant();
150+
assert validate == false || invariant();
146151
}
147152

148153
void update(ShardRouting oldShard, ShardRouting newShard) {
149-
assert invariant();
150154
if (shards.containsKey(oldShard.shardId()) == false) {
151155
// Shard was already removed by routing nodes iterator
152156
// TODO: change caller logic in RoutingNodes so that this check can go away
@@ -174,7 +178,6 @@ void update(ShardRouting oldShard, ShardRouting newShard) {
174178
}
175179

176180
void remove(ShardRouting shard) {
177-
assert invariant();
178181
ShardRouting previousValue = shards.remove(shard.shardId());
179182
assert previousValue == shard : "expected shard " + previousValue + " but was " + shard;
180183
if (shard.initializing()) {
@@ -342,20 +345,24 @@ public boolean isEmpty() {
342345
return shards.isEmpty();
343346
}
344347

345-
private boolean invariant() {
346-
// initializingShards must consistent with that in shards
347-
Collection<ShardRouting> shardRoutingsInitializing = shards.values().stream().filter(ShardRouting::initializing).toList();
348-
assert initializingShards.size() == shardRoutingsInitializing.size();
349-
assert initializingShards.containsAll(shardRoutingsInitializing);
348+
boolean invariant() {
349+
var shardRoutingsInitializing = new ArrayList<ShardRouting>(shards.size());
350+
var shardRoutingsRelocating = new ArrayList<ShardRouting>(shards.size());
351+
// this guess assumes 1 shard per index, this is not precise, but okay for assertion
352+
var shardRoutingsByIndex = Maps.<Index, Set<ShardRouting>>newHashMapWithExpectedSize(shards.size());
350353

351-
// relocatingShards must consistent with that in shards
352-
Collection<ShardRouting> shardRoutingsRelocating = shards.values().stream().filter(ShardRouting::relocating).toList();
353-
assert relocatingShards.size() == shardRoutingsRelocating.size();
354-
assert relocatingShards.containsAll(shardRoutingsRelocating);
354+
for (var shard : shards.values()) {
355+
if (shard.initializing()) {
356+
shardRoutingsInitializing.add(shard);
357+
}
358+
if (shard.relocating()) {
359+
shardRoutingsRelocating.add(shard);
360+
}
361+
shardRoutingsByIndex.computeIfAbsent(shard.index(), k -> new HashSet<>(10)).add(shard);
362+
}
355363

356-
final Map<Index, Set<ShardRouting>> shardRoutingsByIndex = shards.values()
357-
.stream()
358-
.collect(Collectors.groupingBy(ShardRouting::index, Collectors.toSet()));
364+
assert initializingShards.size() == shardRoutingsInitializing.size() && initializingShards.containsAll(shardRoutingsInitializing);
365+
assert relocatingShards.size() == shardRoutingsRelocating.size() && relocatingShards.containsAll(shardRoutingsRelocating);
359366
assert shardRoutingsByIndex.equals(shardsByIndex);
360367

361368
return true;

server/src/main/java/org/elasticsearch/cluster/routing/RoutingNodes.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -122,15 +122,16 @@ private RoutingNodes(RoutingTable routingTable, DiscoveryNodes discoveryNodes, b
122122
// A replica Set might have one (and not more) replicas with the state of RELOCATING.
123123
if (shard.assignedToNode()) {
124124
// LinkedHashMap to preserve order
125-
nodesToShards.computeIfAbsent(shard.currentNodeId(), createRoutingNode).add(shard);
125+
nodesToShards.computeIfAbsent(shard.currentNodeId(), createRoutingNode).addWithoutValidation(shard);
126126
assignedShardsAdd(shard);
127127
if (shard.relocating()) {
128128
relocatingShards++;
129129
ShardRouting targetShardRouting = shard.getTargetRelocatingShard();
130130
addInitialRecovery(targetShardRouting, indexShard.primary);
131131
// LinkedHashMap to preserve order.
132132
// Add the counterpart shard with relocatingNodeId reflecting the source from which it's relocating from.
133-
nodesToShards.computeIfAbsent(shard.relocatingNodeId(), createRoutingNode).add(targetShardRouting);
133+
nodesToShards.computeIfAbsent(shard.relocatingNodeId(), createRoutingNode)
134+
.addWithoutValidation(targetShardRouting);
134135
assignedShardsAdd(targetShardRouting);
135136
} else if (shard.initializing()) {
136137
if (shard.primary()) {
@@ -145,6 +146,7 @@ private RoutingNodes(RoutingTable routingTable, DiscoveryNodes discoveryNodes, b
145146
}
146147
}
147148
}
149+
nodesToShards.values().forEach(RoutingNode::invariant);
148150
}
149151

150152
private RoutingNodes(RoutingNodes routingNodes) {

0 commit comments

Comments
 (0)