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.

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.
+ * <ol>
+ *     <li>First, system indices are ordered before non-system indices</li>
+ *     <li>Then indices are ordered by their priority, in descending order (index.priority)</li>
+ *     <li>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)</li>
+ *     <li>Lastly the index names are compared, which is useful when a date is baked into the index
+ *         name, e.g. <code>logstash-2015.05.03</code></li>
+ * </ol>
  */
 public abstract class PriorityComparator implements Comparator<ShardRouting> {
 
@@ -43,13 +47,20 @@ public abstract class PriorityComparator implements Comparator<ShardRouting> {
         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<ShardRouting> {
         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<ShardRouting> {
     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);
             }
         };
     }

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())) {
+                    settings = buildSettings(10, 100);
+                } else if ("newest".equals(index.getName())) {
+                    settings = buildSettings(100, 1);
+                } else {
+                    settings = 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<ShardRouting> 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())) {
-                    return Settings.builder().put(IndexMetadata.SETTING_CREATION_DATE, 10)
-                            .put(IndexMetadata.SETTING_PRIORITY, 100).build();
+                    settings = buildSettings(10, 100);
+                    isSystem = true;
                 } 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()).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<String, IndexMeta> map = new HashMap<>();
+        IndexMetadata[] indices = new IndexMetadata[numIndices];
+        final Map<String, IndexMetadata> 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();
     }
 }