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
This commit is contained in:
Igor Motov
parent
1b3cd504a5
commit
3354a816fc
|
|
@ -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;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -293,4 +293,27 @@ public class UnassignedInfo implements ToXContent, Writeable<UnassignedInfo> {
|
|||
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;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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> 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
|
||||
*/
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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<String, Object> first, Map<String, Object> second) {
|
||||
public static String differenceBetweenMapsIgnoringArrayOrder(Map<String, Object> first, Map<String, Object> second) {
|
||||
return differenceBetweenMapsIgnoringArrayOrder("", first, second);
|
||||
}
|
||||
|
||||
private static String differenceBetweenMapsIgnoringArrayOrder(String path, Map<String, Object> first, Map<String, Object> 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<Object> secondList = Lists.newArrayList((List<Object>) second);
|
||||
List<Object> firstList = (List<Object>) 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<String, Object>) first, (Map<String, Object>) second);
|
||||
return differenceBetweenMapsIgnoringArrayOrder(path, (Map<String, Object>) first, (Map<String, Object>) 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 + "]";
|
||||
}
|
||||
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue