review comments

Author: ywelsch

core/src/main/java/org/elasticsearch/gateway/PrimaryShardAllocator.java

@@ -257,6 +257,11 @@ private static Map<String, Decision> buildNodeDecisions(NodesToAllocate nodesToA
         return nodeDecisions;
     }
 
+    private static final Comparator<NodeGatewayStartedShards> NO_STORE_EXCEPTION_FIRST_COMPARATOR =
+        Comparator.comparing((NodeGatewayStartedShards state) -> state.storeException() == null).reversed();
+    private static final Comparator<NodeGatewayStartedShards> PRIMARY_FIRST_COMPARATOR =
+        Comparator.comparing(NodeGatewayStartedShards::primary).reversed();
+
     /**
      * Builds a list of nodes. If matchAnyShard is set to false, only nodes that have an allocation id matching
      * inSyncAllocationIds are added to the list. Otherwise, any node that has a shard is added to the list, but
@@ -266,8 +271,7 @@ protected static NodeShardsResult buildAllocationIdBasedNodeShardsResult(ShardRo
                                                                              Set<String> ignoreNodes, Set<String> inSyncAllocationIds,
                                                                              FetchResult<NodeGatewayStartedShards> shardState,
                                                                              Logger logger) {
-        LinkedList<NodeGatewayStartedShards> matchingNodeShardStates = new LinkedList<>();
-        LinkedList<NodeGatewayStartedShards> nonMatchingNodeShardStates = new LinkedList<>();
+        List<NodeGatewayStartedShards> nodeShardStates = new ArrayList<>();
         int numberOfAllocationsFound = 0;
         for (NodeGatewayStartedShards nodeShardState : shardState.getData().values()) {
             DiscoveryNode node = nodeShardState.getNode();
@@ -297,28 +301,27 @@ protected static NodeShardsResult buildAllocationIdBasedNodeShardsResult(ShardRo
             }
 
             if (allocationId != null) {
+                assert nodeShardState.storeException() == null ||
+                    nodeShardState.storeException() instanceof ShardLockObtainFailedException :
+                    "only allow store that can be opened or that throws a ShardLockObtainFailedException while being opened but got a store throwing " + nodeShardState.storeException();
                 numberOfAllocationsFound++;
-                if (inSyncAllocationIds.contains(allocationId)) {
-                    // put shards that were primary before and that didn't throw a ShardLockObtainFailedException first
-                    if (nodeShardState.primary() && nodeShardState.storeException() == null) {
-                        matchingNodeShardStates.addFirst(nodeShardState);
-                    } else {
-                        matchingNodeShardStates.addLast(nodeShardState);
-                    }
-                } else if (matchAnyShard) {
-                    // put shards that were primary before and that didn't throw a ShardLockObtainFailedException first
-                    if (nodeShardState.primary() && nodeShardState.storeException() == null) {
-                        nonMatchingNodeShardStates.addFirst(nodeShardState);
-                    } else {
-                        nonMatchingNodeShardStates.addLast(nodeShardState);
-                    }
+                if (matchAnyShard || inSyncAllocationIds.contains(nodeShardState.allocationId())) {
+                    nodeShardStates.add(nodeShardState);
                 }
             }
         }
 
-        List<NodeGatewayStartedShards> nodeShardStates = new ArrayList<>();
-        nodeShardStates.addAll(matchingNodeShardStates);
-        nodeShardStates.addAll(nonMatchingNodeShardStates);
+        final Comparator<NodeGatewayStartedShards> comparator; // allocation preference
+        if (matchAnyShard) {
+            // prefer shards with matching allocation ids
+            Comparator<NodeGatewayStartedShards> matchingAllocationsFirst = Comparator.comparing(
+                (NodeGatewayStartedShards state) -> inSyncAllocationIds.contains(state.allocationId())).reversed();
+            comparator = matchingAllocationsFirst.thenComparing(NO_STORE_EXCEPTION_FIRST_COMPARATOR).thenComparing(PRIMARY_FIRST_COMPARATOR);
+        } else {
+            comparator = NO_STORE_EXCEPTION_FIRST_COMPARATOR.thenComparing(PRIMARY_FIRST_COMPARATOR);
+        }
+
+        nodeShardStates.sort(comparator);
 
         if (logger.isTraceEnabled()) {
             logger.trace("{} candidates for allocation: {}", shard, nodeShardStates.stream().map(s -> s.getNode().getName()).collect(Collectors.joining(", ")));

core/src/test/java/org/elasticsearch/gateway/PrimaryShardAllocatorTests.java

@@ -202,6 +202,42 @@ public void testShardLockObtainFailedException() {
         assertClusterHealthStatus(allocation, ClusterHealthStatus.YELLOW);
     }
 
+    /**
+     * Tests that when one node returns a ShardLockObtainFailedException and another properly loads the store, it will
+     * select the second node as target
+     */
+    public void testShardLockObtainFailedExceptionPreferOtherValidCopies() {
+        final RoutingAllocation allocation;
+        boolean useAllocationIds = randomBoolean();
+        String allocId1 = randomAsciiOfLength(10);
+        String allocId2 = randomAsciiOfLength(10);
+        if (useAllocationIds) {
+            allocation = routingAllocationWithOnePrimaryNoReplicas(yesAllocationDeciders(), CLUSTER_RECOVERED,
+                randomFrom(Version.V_2_0_0, Version.CURRENT), allocId1, allocId2);
+            testAllocator.addData(node1, ShardStateMetaData.NO_VERSION, allocId1, randomBoolean(),
+                new ShardLockObtainFailedException(shardId, "test"));
+            testAllocator.addData(node2, ShardStateMetaData.NO_VERSION, allocId2, randomBoolean(), null);
+        } else {
+            allocation = routingAllocationWithOnePrimaryNoReplicas(yesAllocationDeciders(), CLUSTER_RECOVERED, Version.V_2_1_1);
+            testAllocator.addData(node1, 3, null, randomBoolean(), new ShardLockObtainFailedException(shardId, "test"));
+            if (randomBoolean()) {
+                testAllocator.addData(node2, randomIntBetween(2, 4), null, randomBoolean(), null);
+            } else {
+                testAllocator.addData(node2, ShardStateMetaData.NO_VERSION, "some alloc id", randomBoolean(), null);
+            }
+        }
+        testAllocator.allocateUnassigned(allocation);
+        assertThat(allocation.routingNodesChanged(), equalTo(true));
+        assertThat(allocation.routingNodes().unassigned().ignored().isEmpty(), equalTo(true));
+        assertThat(allocation.routingNodes().shardsWithState(ShardRoutingState.INITIALIZING).size(), equalTo(1));
+        assertThat(allocation.routingNodes().shardsWithState(ShardRoutingState.INITIALIZING).get(0).currentNodeId(), equalTo(node2.getId()));
+        if (useAllocationIds) {
+            // check that allocation id is reused
+            assertThat(allocation.routingNodes().shardsWithState(ShardRoutingState.INITIALIZING).get(0).allocationId().getId(), equalTo(allocId2));
+        }
+        assertClusterHealthStatus(allocation, ClusterHealthStatus.YELLOW);
+    }
+
     /**
      * Tests that when there is a node to allocate the shard to, it will be allocated to it.
      */