From 80ed3d05bccdf576de983b59101694383f4084e9 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Thu, 19 Dec 2013 19:52:28 +0100 Subject: [PATCH] Use a List of shards per shard ID rather than a set. The shards in the set are mutated after they are added to the set such that the hashcode doesn't fit anymore. For this reason this used an identity hashset before but the downside of this is that the iteration order is not deterministic. We can just use a list since shard removal is a very rare action and the size of the list is very small such that iteration is fast. --- .../cluster/routing/RoutingNodes.java | 61 ++++++++++--------- 1 file changed, 32 insertions(+), 29 deletions(-) diff --git a/src/main/java/org/elasticsearch/cluster/routing/RoutingNodes.java b/src/main/java/org/elasticsearch/cluster/routing/RoutingNodes.java index 97573c0d312..e821d6a1ceb 100644 --- a/src/main/java/org/elasticsearch/cluster/routing/RoutingNodes.java +++ b/src/main/java/org/elasticsearch/cluster/routing/RoutingNodes.java @@ -22,16 +22,12 @@ package org.elasticsearch.cluster.routing; import com.carrotsearch.hppc.ObjectIntOpenHashMap; import com.carrotsearch.hppc.cursors.ObjectCursor; import com.google.common.base.Predicate; -import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterables; -import com.google.common.collect.Iterators; -import com.google.common.collect.Sets; +import com.google.common.collect.*; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.block.ClusterBlocks; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.cluster.node.DiscoveryNode; -import org.elasticsearch.common.collect.IdentityHashSet; import org.elasticsearch.index.shard.ShardId; import java.util.*; @@ -58,7 +54,7 @@ public class RoutingNodes implements Iterable { private final List ignoredUnassignedShards = newArrayList(); - private final Map> assignedShards = newHashMap(); + private final Map> assignedShards = newHashMap(); private int inactivePrimaryCount = 0; @@ -278,14 +274,14 @@ public class RoutingNodes implements Iterable { * ID as the given shard. */ public Iterable assignedShards(ShardRouting shard) { - return Iterables.unmodifiableIterable(assignedShards(shard.shardId())); + return assignedShards(shard.shardId()); } /** * Returns true iff all replicas are active for the given shard routing. Otherwise false */ public boolean allReplicasActive(ShardRouting shardRouting) { - final Set shards = assignedShards(shardRouting.shardId()); + final List shards = assignedShards(shardRouting.shardId()); if (shards.isEmpty() || shards.size() < this.routingTable.index(shardRouting.index()).shard(shardRouting.id()).size()) { return false; // if we are empty nothing is active if we have less than total at least one is unassigned } @@ -427,11 +423,11 @@ public class RoutingNodes implements Iterable { } } - private static final Set EMPTY = Collections.emptySet(); + private static final List EMPTY = Collections.emptyList(); - private Set assignedShards(ShardId shardId) { - final Set replicaSet = assignedShards.get(shardId); - return replicaSet == null ? EMPTY : Collections.unmodifiableSet(replicaSet); + private List assignedShards(ShardId shardId) { + final List replicaSet = assignedShards.get(shardId); + return replicaSet == null ? EMPTY : Collections.unmodifiableList(replicaSet); } /** @@ -457,28 +453,34 @@ public class RoutingNodes implements Iterable { // no unassigned return; } - Set replicaSet = assignedShards.get(shard.shardId()); - if (replicaSet == null) { - replicaSet = new IdentityHashSet(); - assignedShards.put(shard.shardId(), replicaSet); + List shards = assignedShards.get(shard.shardId()); + if (shards == null) { + shards = Lists.newArrayList(); + assignedShards.put(shard.shardId(), shards); } - replicaSet.add(shard); + assert assertInstanceNotInList(shard, shards); + shards.add(shard); + } + + private boolean assertInstanceNotInList(MutableShardRouting shard, List shards) { + for (MutableShardRouting s : shards) { + assert s != shard; + } + return true; } private void assignedShardsRemove(MutableShardRouting shard) { - Set replicaSet = assignedShards.get(shard.shardId()); + final List replicaSet = assignedShards.get(shard.shardId()); if (replicaSet != null) { - if (replicaSet.contains(shard)) { - replicaSet.remove(shard); - } else { - assert false : "Illegal state"; - Iterator iterator = replicaSet.iterator(); - while(iterator.hasNext()) { - if (shard.equals(iterator.next())) { - iterator.remove(); - } + final Iterator iterator = replicaSet.iterator(); + while(iterator.hasNext()) { + // yes we check identity here + if (shard == iterator.next()) { + iterator.remove(); + return; } } + assert false : "Illegal state"; } } @@ -654,7 +656,7 @@ public class RoutingNodes implements Iterable { } // Assert that the active shard routing are identical. Set> entries = indicesAndShards.entrySet(); - Set shards = newHashSet(); + final List shards = newArrayList(); for (Map.Entry e : entries) { String index = e.getKey(); for (int i = 0; i < e.getValue(); i++) { @@ -665,7 +667,8 @@ public class RoutingNodes implements Iterable { } } } - Set mutableShardRoutings = routingNodes.assignedShards(new ShardId(index, i)); + List mutableShardRoutings = routingNodes.assignedShards(new ShardId(index, i)); + assert mutableShardRoutings.size() == shards.size(); for (MutableShardRouting r : mutableShardRoutings) { assert shards.contains(r); shards.remove(r);