From 3f856d1c81af5cb614843f12b1f7e6727fb471c6 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Tue, 22 Sep 2020 15:47:41 +0100 Subject: [PATCH] Prioritise recovery of system index shards (#62640) Closes #61660. When ordering shard for recovery, ensure system index shards are ordered first so that their recovery will be started first. Note that I rewrote PriorityComparatorTests to use IndexMetadata instead of its local IndexMeta POJO. --- .../gateway/PriorityComparator.java | 40 ++-- .../gateway/PriorityComparatorTests.java | 176 ++++++++++++------ 2 files changed, 146 insertions(+), 70 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/gateway/PriorityComparator.java b/server/src/main/java/org/elasticsearch/gateway/PriorityComparator.java index fb27d5fde1a..3b2b31fc6dd 100644 --- a/server/src/main/java/org/elasticsearch/gateway/PriorityComparator.java +++ b/server/src/main/java/org/elasticsearch/gateway/PriorityComparator.java @@ -28,12 +28,16 @@ import org.elasticsearch.index.Index; import java.util.Comparator; /** - * A comparator that compares ShardRouting based on it's indexes priority (index.priority), - * it's creation date (index.creation_date), or eventually by it's index name in reverse order. - * We try to recover first shards from an index with the highest priority, if that's the same - * we try to compare the timestamp the index is created and pick the newer first (time-based indices, - * here the newer indices matter more). If even that is the same, we compare the index name which is useful - * if the date is baked into the index name. ie logstash-2015.05.03. + * A comparator that compares {@link ShardRouting} instances based on various properties. Instances + * are ordered as follows. + *
    + *
  1. First, system indices are ordered before non-system indices
    2. + *
  2. Then indices are ordered by their priority, in descending order (index.priority)
    3. + *
  3. Then newer indices are ordered before older indices, based on their creation date. This benefits + * time-series indices, where newer indices are considered more urgent (index.creation_date)
    4. + *
  4. Lastly the index names are compared, which is useful when a date is baked into the index + * name, e.g. logstash-2015.05.03
    5. + *
*/ public abstract class PriorityComparator implements Comparator { @@ -43,13 +47,20 @@ public abstract class PriorityComparator implements Comparator { final String o2Index = o2.getIndexName(); int cmp = 0; if (o1Index.equals(o2Index) == false) { - final Settings settingsO1 = getIndexSettings(o1.index()); - final Settings settingsO2 = getIndexSettings(o2.index()); - cmp = Long.compare(priority(settingsO2), priority(settingsO1)); + final IndexMetadata metadata01 = getMetadata(o1.index()); + final IndexMetadata metadata02 = getMetadata(o2.index()); + cmp = Boolean.compare(metadata02.isSystem(), metadata01.isSystem()); + if (cmp == 0) { - cmp = Long.compare(timeCreated(settingsO2), timeCreated(settingsO1)); + final Settings settingsO1 = metadata01.getSettings(); + final Settings settingsO2 = metadata02.getSettings(); + cmp = Long.compare(priority(settingsO2), priority(settingsO1)); + if (cmp == 0) { - cmp = o2Index.compareTo(o1Index); + cmp = Long.compare(timeCreated(settingsO2), timeCreated(settingsO1)); + if (cmp == 0) { + cmp = o2Index.compareTo(o1Index); + } } } } @@ -64,7 +75,7 @@ public abstract class PriorityComparator implements Comparator { return settings.getAsLong(IndexMetadata.SETTING_CREATION_DATE, -1L); } - protected abstract Settings getIndexSettings(Index index); + protected abstract IndexMetadata getMetadata(Index index); /** * Returns a PriorityComparator that uses the RoutingAllocation index metadata to access the index setting per index. @@ -72,9 +83,8 @@ public abstract class PriorityComparator implements Comparator { public static PriorityComparator getAllocationComparator(final RoutingAllocation allocation) { return new PriorityComparator() { @Override - protected Settings getIndexSettings(Index index) { - IndexMetadata indexMetadata = allocation.metadata().getIndexSafe(index); - return indexMetadata.getSettings(); + protected IndexMetadata getMetadata(Index index) { + return allocation.metadata().getIndexSafe(index); } }; } diff --git a/server/src/test/java/org/elasticsearch/gateway/PriorityComparatorTests.java b/server/src/test/java/org/elasticsearch/gateway/PriorityComparatorTests.java index ec49c501f84..2fa84b5c549 100644 --- a/server/src/test/java/org/elasticsearch/gateway/PriorityComparatorTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/PriorityComparatorTests.java @@ -18,6 +18,7 @@ */ package org.elasticsearch.gateway; +import org.elasticsearch.Version; import org.elasticsearch.cluster.metadata.IndexMetadata; import org.elasticsearch.cluster.routing.RoutingNodes; import org.elasticsearch.cluster.routing.ShardRouting; @@ -35,6 +36,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; +import static org.hamcrest.Matchers.greaterThan; import static org.mockito.Mockito.mock; public class PriorityComparatorTests extends ESTestCase { @@ -52,15 +54,17 @@ public class PriorityComparatorTests extends ESTestCase { } shards.sort(new PriorityComparator() { @Override - protected Settings getIndexSettings(Index index) { + protected IndexMetadata getMetadata(Index index) { + Settings settings; if ("oldest".equals(index.getName())) { - return Settings.builder().put(IndexMetadata.SETTING_CREATION_DATE, 10) - .put(IndexMetadata.SETTING_PRIORITY, 1).build(); + settings = buildSettings(10, 1); } else if ("newest".equals(index.getName())) { - return Settings.builder().put(IndexMetadata.SETTING_CREATION_DATE, 100) - .put(IndexMetadata.SETTING_PRIORITY, 1).build(); + settings = buildSettings(100, 1); + } else { + settings = Settings.EMPTY; } - return Settings.EMPTY; + + return IndexMetadata.builder(index.getName()).settings(settings).build(); } }); RoutingNodes.UnassignedShards.UnassignedIterator iterator = shards.iterator(); @@ -84,15 +88,53 @@ public class PriorityComparatorTests extends ESTestCase { } shards.sort(new PriorityComparator() { @Override - protected Settings getIndexSettings(Index index) { + protected IndexMetadata getMetadata(Index index) { + Settings settings; if ("oldest".equals(index.getName())) { - return Settings.builder().put(IndexMetadata.SETTING_CREATION_DATE, 10) - .put(IndexMetadata.SETTING_PRIORITY, 100).build(); + settings = buildSettings(10, 100); } else if ("newest".equals(index.getName())) { - return Settings.builder().put(IndexMetadata.SETTING_CREATION_DATE, 100) - .put(IndexMetadata.SETTING_PRIORITY, 1).build(); + settings = buildSettings(100, 1); + } else { + settings = Settings.EMPTY; } - return Settings.EMPTY; + + return IndexMetadata.builder(index.getName()).settings(settings).build(); + } + }); + RoutingNodes.UnassignedShards.UnassignedIterator iterator = shards.iterator(); + ShardRouting next = iterator.next(); + assertEquals("oldest", next.getIndexName()); + next = iterator.next(); + assertEquals("newest", next.getIndexName()); + assertFalse(iterator.hasNext()); + } + + public void testPreferSystemIndices() { + RoutingNodes.UnassignedShards shards = new RoutingNodes.UnassignedShards(mock(RoutingNodes.class)); + List shardRoutings = Arrays.asList( + TestShardRouting.newShardRouting("oldest", 0, null, null, + randomBoolean(), ShardRoutingState.UNASSIGNED, new UnassignedInfo(randomFrom(UnassignedInfo.Reason.values()), "foobar")), + TestShardRouting.newShardRouting("newest", 0, null, null, + randomBoolean(), ShardRoutingState.UNASSIGNED, new UnassignedInfo(randomFrom(UnassignedInfo.Reason.values()), "foobar"))); + Collections.shuffle(shardRoutings, random()); + for (ShardRouting routing : shardRoutings) { + shards.add(routing); + } + shards.sort(new PriorityComparator() { + @Override + protected IndexMetadata getMetadata(Index index) { + Settings settings; + boolean isSystem = false; + if ("oldest".equals(index.getName())) { + settings = buildSettings(10, 100); + isSystem = true; + } else if ("newest".equals(index.getName())) { + settings = buildSettings(100, 1); + } else { + settings = Settings.EMPTY; + } + + return IndexMetadata.builder(index.getName()).system(isSystem).settings(settings).build(); } }); RoutingNodes.UnassignedShards.UnassignedIterator iterator = shards.iterator(); @@ -106,75 +148,99 @@ public class PriorityComparatorTests extends ESTestCase { public void testPriorityComparatorSort() { RoutingNodes.UnassignedShards shards = new RoutingNodes.UnassignedShards(mock(RoutingNodes.class)); int numIndices = randomIntBetween(3, 99); - IndexMeta[] indices = new IndexMeta[numIndices]; - final Map map = new HashMap<>(); + IndexMetadata[] indices = new IndexMetadata[numIndices]; + final Map map = new HashMap<>(); for (int i = 0; i < indices.length; i++) { + int priority = 0; + int creationDate = 0; + boolean isSystem = false; + if (frequently()) { - indices[i] = new IndexMeta("idx_2015_04_" + String.format(Locale.ROOT, "%02d", i), randomIntBetween(1, 1000), - randomIntBetween(1, 10000)); - } else { // sometimes just use defaults - indices[i] = new IndexMeta("idx_2015_04_" + String.format(Locale.ROOT, "%02d", i)); + priority = randomIntBetween(1, 1000); + creationDate = randomIntBetween(1, 10000); } - map.put(indices[i].name, indices[i]); + if (rarely()) { + isSystem = true; + } + // else sometimes just use the defaults + + indices[i] = IndexMetadata.builder(String.format(Locale.ROOT, "idx_%04d", i)) + .system(isSystem) + .settings(buildSettings(creationDate, priority)) + .build(); + + map.put(indices[i].getIndex().getName(), indices[i]); } int numShards = randomIntBetween(10, 100); for (int i = 0; i < numShards; i++) { - IndexMeta indexMeta = randomFrom(indices); - shards.add(TestShardRouting.newShardRouting(indexMeta.name, randomIntBetween(1, 5), null, null, + IndexMetadata indexMeta = randomFrom(indices); + shards.add(TestShardRouting.newShardRouting(indexMeta.getIndex().getName(), randomIntBetween(1, 5), null, null, randomBoolean(), ShardRoutingState.UNASSIGNED, new UnassignedInfo(randomFrom(UnassignedInfo.Reason.values()), "foobar"))); } shards.sort(new PriorityComparator() { @Override - protected Settings getIndexSettings(Index index) { - IndexMeta indexMeta = map.get(index.getName()); - return indexMeta.settings; + protected IndexMetadata getMetadata(Index index) { + return map.get(index.getName()); } }); ShardRouting previous = null; for (ShardRouting routing : shards) { if (previous != null) { - IndexMeta prevMeta = map.get(previous.getIndexName()); - IndexMeta currentMeta = map.get(routing.getIndexName()); - if (prevMeta.priority == currentMeta.priority) { - if (prevMeta.creationDate == currentMeta.creationDate) { - if (prevMeta.name.equals(currentMeta.name) == false) { - assertTrue("indexName mismatch, expected:" + currentMeta.name + " after " + prevMeta.name + " " + - prevMeta.name.compareTo(currentMeta.name), prevMeta.name.compareTo(currentMeta.name) > 0); + IndexMetadata prevMeta = map.get(previous.getIndexName()); + IndexMetadata currentMeta = map.get(routing.getIndexName()); + + if (prevMeta.isSystem() == currentMeta.isSystem()) { + final int prevPriority = prevMeta.getSettings().getAsInt(IndexMetadata.SETTING_PRIORITY, -1); + final int currentPriority = currentMeta.getSettings().getAsInt(IndexMetadata.SETTING_PRIORITY, -1); + + if (prevPriority == currentPriority) { + final int prevCreationDate = prevMeta.getSettings().getAsInt(IndexMetadata.SETTING_CREATION_DATE, -1); + final int currentCreationDate = currentMeta.getSettings().getAsInt(IndexMetadata.SETTING_CREATION_DATE, -1); + + if (prevCreationDate == currentCreationDate) { + final String prevName = prevMeta.getIndex().getName(); + final String currentName = currentMeta.getIndex().getName(); + + if (prevName.equals(currentName) == false) { + assertThat( + "indexName mismatch, expected:" + currentName + " after " + prevName, + prevName, + greaterThan(currentName) + ); + } + } else { + assertThat( + "creationDate mismatch, expected:" + currentCreationDate + " after " + prevCreationDate, + prevCreationDate, greaterThan(currentCreationDate) + ); } } else { - assertTrue("creationDate mismatch, expected:" + currentMeta.creationDate + " after " + prevMeta.creationDate, - prevMeta.creationDate > currentMeta.creationDate); + assertThat( + "priority mismatch, expected:" + currentPriority + " after " + prevPriority, + prevPriority, greaterThan(currentPriority) + ); } } else { - assertTrue("priority mismatch, expected:" + currentMeta.priority + " after " + prevMeta.priority, - prevMeta.priority > currentMeta.priority); + assertThat( + "system mismatch, expected:" + currentMeta.isSystem() + " after " + prevMeta.isSystem(), + prevMeta.isSystem(), + greaterThan(currentMeta.isSystem()) + ); } } previous = routing; } } - private static class IndexMeta { - final String name; - final int priority; - final long creationDate; - final Settings settings; - - private IndexMeta(String name) { // default - this.name = name; - this.priority = 1; - this.creationDate = -1; - this.settings = Settings.EMPTY; - } - - private IndexMeta(String name, int priority, long creationDate) { - this.name = name; - this.priority = priority; - this.creationDate = creationDate; - this.settings = Settings.builder().put(IndexMetadata.SETTING_CREATION_DATE, creationDate) - .put(IndexMetadata.SETTING_PRIORITY, priority).build(); - } + private static Settings buildSettings(int creationDate, int priority) { + return Settings.builder() + .put(IndexMetadata.SETTING_CREATION_DATE, creationDate) + .put(IndexMetadata.SETTING_PRIORITY, priority) + .put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT) + .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) + .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0) + .build(); } }