Skip to content
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

Speedup BalanceUnbalancedClusterTests #88794

Merged
merged 2 commits into from
Jul 28, 2022
Merged

Speedup BalanceUnbalancedClusterTests #88794

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b1f94a
Speedup BalanceUnbalancedClusterTests
idegtiarenko Jul 26, 2022
bd2ffc9
upd
idegtiarenko Jul 28, 2022
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
43 changes: 25 additions & 18 deletions server/src/main/java/org/elasticsearch/cluster/routing/RoutingNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import org.elasticsearch.index.shard.ShardId;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.Iterator;
Expand All @@ -26,7 +25,6 @@
import java.util.Objects;
import java.util.Set;
import java.util.function.Predicate;
import java.util.stream.Collectors;

/**
* A {@link RoutingNode} represents a cluster node associated with a single {@link DiscoveryNode} including all shards
Expand Down Expand Up @@ -118,7 +116,14 @@ public int size() {
* @param shard Shard to create on this Node
*/
void add(ShardRouting shard) {
assert invariant();
addInternal(shard, true);
}

void addNoValidate(ShardRouting shard) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about addWithoutValidation? It sounds more natural for me.

addInternal(shard, false);
}

private void addInternal(ShardRouting shard, boolean validate) {
final ShardRouting existing = shards.putIfAbsent(shard.shardId(), shard);
if (existing != null) {
final IllegalStateException e = new IllegalStateException(
Expand All @@ -142,11 +147,10 @@ void add(ShardRouting shard) {
relocatingShards.add(shard);
}
shardsByIndex.computeIfAbsent(shard.index(), k -> new HashSet<>()).add(shard);
assert invariant();
assert validate == false || invariant();
}

void update(ShardRouting oldShard, ShardRouting newShard) {
assert invariant();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The invariant asserted after operation as well. I do not think it is required to do it twice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

if (shards.containsKey(oldShard.shardId()) == false) {
// Shard was already removed by routing nodes iterator
// TODO: change caller logic in RoutingNodes so that this check can go away
Expand Down Expand Up @@ -174,7 +178,6 @@ void update(ShardRouting oldShard, ShardRouting newShard) {
}

void remove(ShardRouting shard) {
assert invariant();
ShardRouting previousValue = shards.remove(shard.shardId());
assert previousValue == shard : "expected shard " + previousValue + " but was " + shard;
if (shard.initializing()) {
Expand Down Expand Up @@ -342,20 +345,24 @@ public boolean isEmpty() {
return shards.isEmpty();
}

private boolean invariant() {
// initializingShards must consistent with that in shards
Collection<ShardRouting> shardRoutingsInitializing = shards.values().stream().filter(ShardRouting::initializing).toList();
assert initializingShards.size() == shardRoutingsInitializing.size();
assert initializingShards.containsAll(shardRoutingsInitializing);
boolean invariant() {
var shardRoutingsInitializing = new ArrayList<ShardRouting>(shards.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about declaring shardRoutingsInitializing and shardRoutingsRelocating as HashSet? Then we can do assertions as simple equals calls

assert initializingShards.equals(shardRoutingsInitializing);
assert relocatingShards.equals(shardRoutingsRelocating);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about it, but then decided to keep Lists to avoid calculating hash for each item.

Copy link
Contributor

Choose a reason for hiding this comment

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

hashCode is cached for ShardRouting, but I agree that it does seem redundant is this case, bceause these new collections are never queried.

var shardRoutingsRelocating = new ArrayList<ShardRouting>(shards.size());
// this guess assumes 1 shard per index, this is not precise, but okay for assertion
var shardRoutingsByIndex = Maps.<Index, Set<ShardRouting>>newHashMapWithExpectedSize(shards.size());

// relocatingShards must consistent with that in shards
Collection<ShardRouting> shardRoutingsRelocating = shards.values().stream().filter(ShardRouting::relocating).toList();
assert relocatingShards.size() == shardRoutingsRelocating.size();
assert relocatingShards.containsAll(shardRoutingsRelocating);
for (var shard : shards.values()) {
if (shard.initializing()) {
shardRoutingsInitializing.add(shard);
}
if (shard.relocating()) {
shardRoutingsRelocating.add(shard);
}
shardRoutingsByIndex.computeIfAbsent(shard.index(), k -> new HashSet<>(10)).add(shard);
Copy link
Contributor

@arteam arteam Jul 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Does it make sense to use Sets.newHashSetWithExpectedSize here to correctly pre-size the HashSet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately we do not know expected size here. I used some random number to avoid initial resize(s)

}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was rebuild for assertion twice after any shard addition to the routing node.
This was especially slow for a test with 4k shards.


final Map<Index, Set<ShardRouting>> shardRoutingsByIndex = shards.values()
.stream()
.collect(Collectors.groupingBy(ShardRouting::index, Collectors.toSet()));
assert initializingShards.size() == shardRoutingsInitializing.size() && initializingShards.containsAll(shardRoutingsInitializing);
assert relocatingShards.size() == shardRoutingsRelocating.size() && relocatingShards.containsAll(shardRoutingsRelocating);
assert shardRoutingsByIndex.equals(shardsByIndex);

return true;
Expand Down
7 changes: 5 additions & 2 deletions server/src/main/java/org/elasticsearch/cluster/routing/RoutingNodes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,15 +122,15 @@ private RoutingNodes(RoutingTable routingTable, DiscoveryNodes discoveryNodes, b
// A replica Set might have one (and not more) replicas with the state of RELOCATING.
if (shard.assignedToNode()) {
// LinkedHashMap to preserve order
nodesToShards.computeIfAbsent(shard.currentNodeId(), createRoutingNode).add(shard);
nodesToShards.computeIfAbsent(shard.currentNodeId(), createRoutingNode).addNoValidate(shard);
assignedShardsAdd(shard);
if (shard.relocating()) {
relocatingShards++;
ShardRouting targetShardRouting = shard.getTargetRelocatingShard();
addInitialRecovery(targetShardRouting, indexShard.primary);
// LinkedHashMap to preserve order.
// Add the counterpart shard with relocatingNodeId reflecting the source from which it's relocating from.
nodesToShards.computeIfAbsent(shard.relocatingNodeId(), createRoutingNode).add(targetShardRouting);
nodesToShards.computeIfAbsent(shard.relocatingNodeId(), createRoutingNode).addNoValidate(targetShardRouting);
assignedShardsAdd(targetShardRouting);
} else if (shard.initializing()) {
if (shard.primary()) {
Expand All @@ -145,6 +145,9 @@ private RoutingNodes(RoutingTable routingTable, DiscoveryNodes discoveryNodes, b
}
}
}
for (var node : nodesToShards.values()) {
assert node.invariant();
}
Copy link
Contributor Author

@idegtiarenko idegtiarenko Jul 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Invariant checking is expensive in case of big collections so it is verified once in the end instead of after every operation.

This is constructing RoutingNodes from RoutingTable so no other concurrent additions is expected

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. One suggestion: I think we do the invariant check in one line with method references:

nodesToShards.values().forEach(RoutingNode::invariant)

Copy link
Member

Choose a reason for hiding this comment

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

Link #88951 :)

}

private RoutingNodes(RoutingNodes routingNodes) {
Expand Down