Tie-break shard path decision based on total number of shards on path (…

…#27039) Right now if the number of shards for a particular index is equal across the data paths, we tie-break on space. This changes to tie-break first on the total number of shards for each path, and then, if that is the same, on the usable bytes. Relates to #26654 (it's a follow-up)
elastic · Oct 24, 2017 · e660c6f · e660c6f
1 parent 7adfe8e
commit e660c6f
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 2 deletions.
diff --git a/core/src/main/java/org/elasticsearch/index/shard/ShardPath.java b/core/src/main/java/org/elasticsearch/index/shard/ShardPath.java
@@ -222,8 +222,13 @@ public static ShardPath selectNewPathForShard(NodeEnvironment env, ShardId shard
                         .sorted((p1, p2) -> {
                                 int cmp = Long.compare(pathToShardCount.getOrDefault(p1, 0L), pathToShardCount.getOrDefault(p2, 0L));
                                 if (cmp == 0) {
-                                    // if the number of shards is equal, tie-break with the usable bytes
-                                    cmp = pathsToSpace.get(p2).compareTo(pathsToSpace.get(p1));
+                                    // if the number of shards is equal, tie-break with the number of total shards
+                                    cmp = Integer.compare(dataPathToShardCount.getOrDefault(p1.path, 0),
+                                            dataPathToShardCount.getOrDefault(p2.path, 0));
+                                    if (cmp == 0) {
+                                        // if the number of shards is equal, tie-break with the usable bytes
+                                        cmp = pathsToSpace.get(p2).compareTo(pathsToSpace.get(p1));
+                                    }
                                 }
                                 return cmp;
                             })

diff --git a/core/src/test/java/org/elasticsearch/index/shard/NewPathForShardTests.java b/core/src/test/java/org/elasticsearch/index/shard/NewPathForShardTests.java
@@ -304,4 +304,71 @@ public void testGettingPathWithMostFreeSpace() throws Exception {
 
         nodeEnv.close();
     }
+
+    public void testTieBreakWithMostShards() throws Exception {
+        Path path = PathUtils.get(createTempDir().toString());
+
+        // Use 2 data paths:
+        String[] paths = new String[] {path.resolve("a").toString(),
+                                       path.resolve("b").toString()};
+
+        Settings settings = Settings.builder()
+            .put(Environment.PATH_HOME_SETTING.getKey(), path)
+            .putList(Environment.PATH_DATA_SETTING.getKey(), paths).build();
+        NodeEnvironment nodeEnv = new NodeEnvironment(settings, new Environment(settings));
+
+        // Make sure all our mocking above actually worked:
+        NodePath[] nodePaths = nodeEnv.nodePaths();
+        assertEquals(2, nodePaths.length);
+
+        assertEquals("mocka", nodePaths[0].fileStore.name());
+        assertEquals("mockb", nodePaths[1].fileStore.name());
+
+        // Path a has lots of free space, but b has little, so new shard should go to a:
+        aFileStore.usableSpace = 100000;
+        bFileStore.usableSpace = 10000;
+
+        Map<Path, Integer> dataPathToShardCount = new HashMap<>();
+
+        ShardId shardId = new ShardId("index", "uid1", 0);
+        ShardPath result = ShardPath.selectNewPathForShard(nodeEnv, shardId, INDEX_SETTINGS, 100, dataPathToShardCount);
+        createFakeShard(result);
+        // First shard should go to a
+        assertThat(result.getDataPath().toString(), containsString(aPathPart));
+        dataPathToShardCount.compute(NodeEnvironment.shardStatePathToDataPath(result.getDataPath()), (k, v) -> v == null ? 1 : v + 1);
+
+        shardId = new ShardId("index", "uid1", 1);
+        result = ShardPath.selectNewPathForShard(nodeEnv, shardId, INDEX_SETTINGS, 100, dataPathToShardCount);
+        createFakeShard(result);
+        // Second shard should go to b
+        assertThat(result.getDataPath().toString(), containsString(bPathPart));
+        dataPathToShardCount.compute(NodeEnvironment.shardStatePathToDataPath(result.getDataPath()), (k, v) -> v == null ? 1 : v + 1);
+
+        shardId = new ShardId("index2", "uid3", 0);
+        result = ShardPath.selectNewPathForShard(nodeEnv, shardId, INDEX_SETTINGS, 100, dataPathToShardCount);
+        createFakeShard(result);
+        // Shard for new index should go to a
+        assertThat(result.getDataPath().toString(), containsString(aPathPart));
+        dataPathToShardCount.compute(NodeEnvironment.shardStatePathToDataPath(result.getDataPath()), (k, v) -> v == null ? 1 : v + 1);
+
+        shardId = new ShardId("index2", "uid2", 0);
+        IndexSettings idxSettings = IndexSettingsModule.newIndexSettings("index2",
+                Settings.builder().put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 3).build());
+        ShardPath result1 = ShardPath.selectNewPathForShard(nodeEnv, shardId, idxSettings, 100, dataPathToShardCount);
+        createFakeShard(result1);
+        dataPathToShardCount.compute(NodeEnvironment.shardStatePathToDataPath(result1.getDataPath()), (k, v) -> v == null ? 1 : v + 1);
+        shardId = new ShardId("index2", "uid2", 1);
+        ShardPath result2 = ShardPath.selectNewPathForShard(nodeEnv, shardId, idxSettings, 100, dataPathToShardCount);
+        createFakeShard(result2);
+        dataPathToShardCount.compute(NodeEnvironment.shardStatePathToDataPath(result2.getDataPath()), (k, v) -> v == null ? 1 : v + 1);
+        shardId = new ShardId("index2", "uid2", 2);
+        ShardPath result3 = ShardPath.selectNewPathForShard(nodeEnv, shardId, idxSettings, 100, dataPathToShardCount);
+        createFakeShard(result3);
+        // 2 shards go to 'b' and 1 to 'a'
+        assertThat(result1.getDataPath().toString(), containsString(bPathPart));
+        assertThat(result2.getDataPath().toString(), containsString(aPathPart));
+        assertThat(result3.getDataPath().toString(), containsString(bPathPart));
+
+        nodeEnv.close();
+    }
 }