From 3354a816fca4b8ce5af622c848bcd5fd1161e3b7 Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Fri, 7 Aug 2015 17:09:08 -0400 Subject: [PATCH] Changes in unassigned info and version might not be transferred as part of cluster state diffs The equalTo logic of ShardRouting doesn't take version and unassignedInfo into the account when compares shard routings. Since cluster state diff relies on equal to detect the changes that needs to be sent to other cluster, this omission might lead to changes not being properly propagated to other nodes in the cluster. Closes #12387 --- .../cluster/routing/ShardRouting.java | 9 +- .../cluster/routing/UnassignedInfo.java | 23 +++++ .../cluster/ClusterStateDiffIT.java | 30 ++++++- .../routing/RandomShardRoutingMutator.java | 88 +++++++++++++++++++ .../elasticsearch/test/ESIntegTestCase.java | 5 +- .../elasticsearch/test/XContentTestUtils.java | 58 ++++++++---- 6 files changed, 187 insertions(+), 26 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/cluster/routing/RandomShardRoutingMutator.java diff --git a/core/src/main/java/org/elasticsearch/cluster/routing/ShardRouting.java b/core/src/main/java/org/elasticsearch/cluster/routing/ShardRouting.java index e1b499fc606..9c0af60a16f 100644 --- a/core/src/main/java/org/elasticsearch/cluster/routing/ShardRouting.java +++ b/core/src/main/java/org/elasticsearch/cluster/routing/ShardRouting.java @@ -608,7 +608,12 @@ public final class ShardRouting implements Streamable, ToXContent { return false; } ShardRouting that = (ShardRouting) o; - // TODO: add version + unassigned info check. see #12387 + if (version != that.version) { + return false; + } + if (unassignedInfo != null ? !unassignedInfo.equals(that.unassignedInfo) : that.unassignedInfo != null) { + return false; + } return equalsIgnoringMetaData(that); } @@ -626,8 +631,10 @@ public final class ShardRouting implements Streamable, ToXContent { result = 31 * result + (relocatingNodeId != null ? relocatingNodeId.hashCode() : 0); result = 31 * result + (primary ? 1 : 0); result = 31 * result + (state != null ? state.hashCode() : 0); + result = 31 * result + (int) (version ^ (version >>> 32)); result = 31 * result + (restoreSource != null ? restoreSource.hashCode() : 0); result = 31 * result + (allocationId != null ? allocationId.hashCode() : 0); + result = 31 * result + (unassignedInfo != null ? unassignedInfo.hashCode() : 0); return hashCode = result; } diff --git a/core/src/main/java/org/elasticsearch/cluster/routing/UnassignedInfo.java b/core/src/main/java/org/elasticsearch/cluster/routing/UnassignedInfo.java index 1c602491f26..e6dc7184cc3 100644 --- a/core/src/main/java/org/elasticsearch/cluster/routing/UnassignedInfo.java +++ b/core/src/main/java/org/elasticsearch/cluster/routing/UnassignedInfo.java @@ -293,4 +293,27 @@ public class UnassignedInfo implements ToXContent, Writeable { builder.endObject(); return builder; } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + + UnassignedInfo that = (UnassignedInfo) o; + + if (timestamp != that.timestamp) return false; + if (reason != that.reason) return false; + if (message != null ? !message.equals(that.message) : that.message != null) return false; + return !(failure != null ? !failure.equals(that.failure) : that.failure != null); + + } + + @Override + public int hashCode() { + int result = reason != null ? reason.hashCode() : 0; + result = 31 * result + (int) (timestamp ^ (timestamp >>> 32)); + result = 31 * result + (message != null ? message.hashCode() : 0); + result = 31 * result + (failure != null ? failure.hashCode() : 0); + return result; + } } diff --git a/core/src/test/java/org/elasticsearch/cluster/ClusterStateDiffIT.java b/core/src/test/java/org/elasticsearch/cluster/ClusterStateDiffIT.java index a33730b8342..edd6254b511 100644 --- a/core/src/test/java/org/elasticsearch/cluster/ClusterStateDiffIT.java +++ b/core/src/test/java/org/elasticsearch/cluster/ClusterStateDiffIT.java @@ -47,8 +47,10 @@ import org.junit.Test; import java.util.List; import static org.elasticsearch.cluster.metadata.AliasMetaData.newAliasMetaDataBuilder; +import static org.elasticsearch.cluster.routing.RandomShardRoutingMutator.randomChange; +import static org.elasticsearch.cluster.routing.RandomShardRoutingMutator.randomReason; import static org.elasticsearch.test.XContentTestUtils.convertToMap; -import static org.elasticsearch.test.XContentTestUtils.mapsEqualIgnoringArrayOrder; +import static org.elasticsearch.test.XContentTestUtils.differenceBetweenMapsIgnoringArrayOrder; import static org.elasticsearch.test.VersionUtils.randomVersion; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; @@ -151,7 +153,7 @@ public class ClusterStateDiffIT extends ESIntegTestCase { assertThat(clusterStateFromDiffs.metaData().equalsAliases(clusterState.metaData()), is(true)); // JSON Serialization test - make sure that both states produce similar JSON - assertThat(mapsEqualIgnoringArrayOrder(convertToMap(clusterStateFromDiffs), convertToMap(clusterState)), equalTo(true)); + assertNull(differenceBetweenMapsIgnoringArrayOrder(convertToMap(clusterStateFromDiffs), convertToMap(clusterState))); // Smoke test - we cannot compare bytes to bytes because some elements might get serialized in different order // however, serialized size should remain the same @@ -200,7 +202,7 @@ public class ClusterStateDiffIT extends ESIntegTestCase { if (randomBoolean()) { builder.remove(index); } else { - builder.add(randomIndexRoutingTable(index, clusterState.nodes().nodes().keys().toArray(String.class))); + builder.add(randomChangeToIndexRoutingTable(clusterState.routingTable().indicesRouting().get(index), clusterState.nodes().nodes().keys().toArray(String.class))); } } } @@ -222,14 +224,34 @@ public class ClusterStateDiffIT extends ESIntegTestCase { IndexShardRoutingTable.Builder indexShard = new IndexShardRoutingTable.Builder(new ShardId(index, i)); int replicaCount = randomIntBetween(1, 10); for (int j = 0; j < replicaCount; j++) { + UnassignedInfo unassignedInfo = null; + if (randomInt(5) == 1) { + unassignedInfo = new UnassignedInfo(randomReason(), randomAsciiOfLength(10)); + } indexShard.addShard( - TestShardRouting.newShardRouting(index, i, randomFrom(nodeIds), null, null, j == 0, ShardRoutingState.fromValue((byte) randomIntBetween(2, 4)), 1)); + TestShardRouting.newShardRouting(index, i, randomFrom(nodeIds), null, null, j == 0, + ShardRoutingState.fromValue((byte) randomIntBetween(2, 4)), 1, unassignedInfo)); } builder.addIndexShard(indexShard.build()); } return builder.build(); } + /** + * Randomly updates index routing table in the cluster state + */ + private IndexRoutingTable randomChangeToIndexRoutingTable(IndexRoutingTable original, String[] nodes) { + IndexRoutingTable.Builder builder = IndexRoutingTable.builder(original.getIndex()); + for (ObjectCursor indexShardRoutingTable : original.shards().values()) { + for (ShardRouting shardRouting : indexShardRoutingTable.value.shards()) { + final ShardRouting newShardRouting = new ShardRouting(shardRouting); + randomChange(newShardRouting, nodes); + builder.addShard(indexShardRoutingTable.value, newShardRouting); + } + } + return builder.build(); + } + /** * Randomly creates or removes cluster blocks */ diff --git a/core/src/test/java/org/elasticsearch/cluster/routing/RandomShardRoutingMutator.java b/core/src/test/java/org/elasticsearch/cluster/routing/RandomShardRoutingMutator.java new file mode 100644 index 00000000000..4ca849d8ae4 --- /dev/null +++ b/core/src/test/java/org/elasticsearch/cluster/routing/RandomShardRoutingMutator.java @@ -0,0 +1,88 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.cluster.routing; + +import static org.elasticsearch.test.ESTestCase.randomAsciiOfLength; +import static org.elasticsearch.test.ESTestCase.randomFrom; +import static org.elasticsearch.test.ESTestCase.randomInt; + +/** + * Utility class the makes random modifications to ShardRouting + */ +public final class RandomShardRoutingMutator { + private RandomShardRoutingMutator() { + + } + + public static void randomChange(ShardRouting shardRouting, String[] nodes) { + switch (randomInt(3)) { + case 0: + if (shardRouting.unassigned() == false) { + shardRouting.moveToUnassigned(new UnassignedInfo(randomReason(), randomAsciiOfLength(10))); + } else if (shardRouting.unassignedInfo() != null) { + shardRouting.updateUnassignedInfo(new UnassignedInfo(randomReason(), randomAsciiOfLength(10))); + } + break; + case 1: + if (shardRouting.unassigned()) { + shardRouting.initialize(randomFrom(nodes)); + } + break; + case 2: + if (shardRouting.primary()) { + shardRouting.moveFromPrimary(); + } else { + shardRouting.moveToPrimary(); + } + break; + case 3: + if (shardRouting.initializing()) { + shardRouting.moveToStarted(); + } + break; + } + } + + + public static UnassignedInfo.Reason randomReason() { + switch (randomInt(9)) { + case 0: + return UnassignedInfo.Reason.INDEX_CREATED; + case 1: + return UnassignedInfo.Reason.CLUSTER_RECOVERED; + case 2: + return UnassignedInfo.Reason.INDEX_REOPENED; + case 3: + return UnassignedInfo.Reason.DANGLING_INDEX_IMPORTED; + case 4: + return UnassignedInfo.Reason.NEW_INDEX_RESTORED; + case 5: + return UnassignedInfo.Reason.EXISTING_INDEX_RESTORED; + case 6: + return UnassignedInfo.Reason.REPLICA_ADDED; + case 7: + return UnassignedInfo.Reason.ALLOCATION_FAILED; + case 8: + return UnassignedInfo.Reason.NODE_LEFT; + default: + return UnassignedInfo.Reason.REROUTE_CANCELLED; + } + } +} diff --git a/core/src/test/java/org/elasticsearch/test/ESIntegTestCase.java b/core/src/test/java/org/elasticsearch/test/ESIntegTestCase.java index 0784d2a75f3..8d4b7c4cb51 100644 --- a/core/src/test/java/org/elasticsearch/test/ESIntegTestCase.java +++ b/core/src/test/java/org/elasticsearch/test/ESIntegTestCase.java @@ -67,7 +67,6 @@ import org.elasticsearch.client.Client; import org.elasticsearch.client.Requests; import org.elasticsearch.cluster.ClusterService; import org.elasticsearch.cluster.ClusterState; -import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.cluster.metadata.MappingMetaData; import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.cluster.routing.IndexRoutingTable; @@ -140,7 +139,7 @@ import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF import static org.elasticsearch.common.settings.Settings.settingsBuilder; import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery; import static org.elasticsearch.test.XContentTestUtils.convertToMap; -import static org.elasticsearch.test.XContentTestUtils.mapsEqualIgnoringArrayOrder; +import static org.elasticsearch.test.XContentTestUtils.differenceBetweenMapsIgnoringArrayOrder; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.*; import static org.hamcrest.Matchers.*; @@ -1072,7 +1071,7 @@ public abstract class ESIntegTestCase extends ESTestCase { // but we can compare serialization sizes - they should be the same assertEquals("clusterstate size does not match", masterClusterStateSize, localClusterStateSize); // Compare JSON serialization - assertTrue("clusterstate JSON serialization does not match", mapsEqualIgnoringArrayOrder(masterStateMap, localStateMap)); + assertNull("clusterstate JSON serialization does not match", differenceBetweenMapsIgnoringArrayOrder(masterStateMap, localStateMap)); } catch (AssertionError error) { logger.error("Cluster state from master:\n{}\nLocal cluster state:\n{}", masterClusterState.toString(), localClusterState.toString()); throw error; diff --git a/core/src/test/java/org/elasticsearch/test/XContentTestUtils.java b/core/src/test/java/org/elasticsearch/test/XContentTestUtils.java index 917c66a452b..647930398b6 100644 --- a/core/src/test/java/org/elasticsearch/test/XContentTestUtils.java +++ b/core/src/test/java/org/elasticsearch/test/XContentTestUtils.java @@ -46,58 +46,80 @@ public final class XContentTestUtils { /** - * Compares to maps generated from XContentObjects. The order of elements in arrays is ignored + * Compares to maps generated from XContentObjects. The order of elements in arrays is ignored. + * + * @return null if maps are equal or path to the element where the difference was found */ - public static boolean mapsEqualIgnoringArrayOrder(Map first, Map second) { + public static String differenceBetweenMapsIgnoringArrayOrder(Map first, Map second) { + return differenceBetweenMapsIgnoringArrayOrder("", first, second); + } + + private static String differenceBetweenMapsIgnoringArrayOrder(String path, Map first, Map second) { if (first.size() != second.size()) { - return false; + return path + ": sizes of the maps don't match: " + first.size() + " != " + second.size(); } for (String key : first.keySet()) { - if (objectsEqualIgnoringArrayOrder(first.get(key), second.get(key)) == false) { - return false; + String reason = differenceBetweenObjectsIgnoringArrayOrder(path + "/" + key, first.get(key), second.get(key)); + if (reason != null) { + return reason; } } - return true; + return null; } @SuppressWarnings("unchecked") - private static boolean objectsEqualIgnoringArrayOrder(Object first, Object second) { - if (first == null ) { - return second == null; + private static String differenceBetweenObjectsIgnoringArrayOrder(String path, Object first, Object second) { + if (first == null) { + if (second == null) { + return null; + } else { + return path + ": first element is null, the second element is not null"; + } } else if (first instanceof List) { - if (second instanceof List) { + if (second instanceof List) { List secondList = Lists.newArrayList((List) second); List firstList = (List) first; if (firstList.size() == secondList.size()) { + String reason = path + ": no matches found"; for (Object firstObj : firstList) { boolean found = false; for (Object secondObj : secondList) { - if (objectsEqualIgnoringArrayOrder(firstObj, secondObj)) { + reason = differenceBetweenObjectsIgnoringArrayOrder(path + "/*", firstObj, secondObj); + if (reason == null) { secondList.remove(secondObj); found = true; break; } } if (found == false) { - return false; + return reason; } } - return secondList.isEmpty(); + if (secondList.isEmpty()) { + return null; + } else { + return path + ": the second list is not empty"; + } } else { - return false; + return path + ": sizes of the arrays don't match: " + firstList.size() + " != " + secondList.size(); } } else { - return false; + return path + ": the second element is not an array"; } } else if (first instanceof Map) { if (second instanceof Map) { - return mapsEqualIgnoringArrayOrder((Map) first, (Map) second); + return differenceBetweenMapsIgnoringArrayOrder(path, (Map) first, (Map) second); } else { - return false; + return path + ": the second element is not a map"; } } else { - return first.equals(second); + if (first.equals(second)) { + return null; + } else { + return path + ": the elements don't match: [" + first + "] != [" + second + "]"; + } + } }