From 25abf42e27cb23940e1a76c931dd66fe0b39c621 Mon Sep 17 00:00:00 2001 From: Harsh Garg Date: Mon, 27 May 2024 11:51:08 +0530 Subject: [PATCH 1/6] Optimizing get indices to segments map calculations for IndicesSegmentResponse class Signed-off-by: Harsh Garg --- .../segments/IndicesSegmentResponse.java | 23 ++++++++----------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java b/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java index 7de4cd1702910..6adc5c3a0d35c 100644 --- a/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java +++ b/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java @@ -47,11 +47,9 @@ import java.io.IOException; import java.util.ArrayList; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Map; -import java.util.Set; /** * Transport response for retrieving indices segment information @@ -87,21 +85,18 @@ public Map getIndices() { } Map indicesSegments = new HashMap<>(); - Set indices = new HashSet<>(); + Map> indicesShardSegments = new HashMap<>(); for (ShardSegments shard : shards) { - indices.add(shard.getShardRouting().getIndexName()); - } - - for (String indexName : indices) { - List shards = new ArrayList<>(); - for (ShardSegments shard : this.shards) { - if (shard.getShardRouting().getIndexName().equals(indexName)) { - shards.add(shard); - } + List shardSegList = indicesShardSegments.get(shard.getShardRouting().getIndexName()); + if (shardSegList == null) { + shardSegList = new ArrayList<>(); + indicesShardSegments.put(shard.getShardRouting().getIndexName(), shardSegList); } - indicesSegments.put(indexName, new IndexSegments(indexName, shards.toArray(new ShardSegments[shards.size()]))); + shardSegList.add(shard); + } + for (Map.Entry> entry : indicesShardSegments.entrySet()) { + indicesSegments.put(entry.getKey(), new IndexSegments(entry.getKey(), entry.getValue().toArray(new ShardSegments[0]))); } - this.indicesSegments = indicesSegments; return indicesSegments; } From a547a92fc78b089e871355e84f6e7c36076a18a5 Mon Sep 17 00:00:00 2001 From: Harsh Garg Date: Mon, 19 Aug 2024 10:28:04 +0530 Subject: [PATCH 2/6] Avoid creating new map for storing index to shardsSegments Signed-off-by: Harsh Garg --- .../segments/IndicesSegmentResponse.java | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java b/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java index 6adc5c3a0d35c..631854e69fd31 100644 --- a/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java +++ b/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java @@ -45,7 +45,8 @@ import org.opensearch.index.engine.Segment; import java.io.IOException; -import java.util.ArrayList; +import java.util.Arrays; +import java.util.Comparator; import java.util.HashMap; import java.util.List; import java.util.Locale; @@ -83,19 +84,21 @@ public Map getIndices() { if (indicesSegments != null) { return indicesSegments; } + + Arrays.sort(shards, Comparator.comparing(shardSegment -> shardSegment.getShardRouting().getIndexName())); Map indicesSegments = new HashMap<>(); - Map> indicesShardSegments = new HashMap<>(); - for (ShardSegments shard : shards) { - List shardSegList = indicesShardSegments.get(shard.getShardRouting().getIndexName()); - if (shardSegList == null) { - shardSegList = new ArrayList<>(); - indicesShardSegments.put(shard.getShardRouting().getIndexName(), shardSegList); + int lastIdx = 0; + for (int i = 0; i < shards.length; i++) { + String lastIndexName = shards[lastIdx].getShardRouting().getIndexName(); + String currentIndexName = shards[i].getShardRouting().getIndexName(); + if (!currentIndexName.equals(lastIndexName)) { + indicesSegments.put(lastIndexName, new IndexSegments(lastIndexName, Arrays.copyOfRange(shards, lastIdx, i))); + lastIdx = i; + } + if (i == shards.length - 1) { + indicesSegments.put(currentIndexName, new IndexSegments(currentIndexName, Arrays.copyOfRange(shards, lastIdx, i + 1))); } - shardSegList.add(shard); - } - for (Map.Entry> entry : indicesShardSegments.entrySet()) { - indicesSegments.put(entry.getKey(), new IndexSegments(entry.getKey(), entry.getValue().toArray(new ShardSegments[0]))); } return indicesSegments; } From 8e4aadf930f4998c9098cda630e6825ad450d11e Mon Sep 17 00:00:00 2001 From: Harsh Garg Date: Fri, 30 Aug 2024 09:47:56 +0530 Subject: [PATCH 3/6] Fixing variable names Signed-off-by: Harsh Garg --- .../segments/IndicesSegmentResponse.java | 32 ++++++++++++------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java b/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java index 631854e69fd31..ad37887ed947a 100644 --- a/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java +++ b/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java @@ -84,22 +84,32 @@ public Map getIndices() { if (indicesSegments != null) { return indicesSegments; } - - Arrays.sort(shards, Comparator.comparing(shardSegment -> shardSegment.getShardRouting().getIndexName())); Map indicesSegments = new HashMap<>(); + if (shards.length == 0) { + this.indicesSegments = indicesSegments; + return indicesSegments; + } - int lastIdx = 0; + Arrays.sort(shards, Comparator.comparing(shardSegment -> shardSegment.getShardRouting().getIndexName())); + int startIndexPos = 0; + String startIndexName = shards[startIndexPos].getShardRouting().getIndexName(); for (int i = 0; i < shards.length; i++) { - String lastIndexName = shards[lastIdx].getShardRouting().getIndexName(); - String currentIndexName = shards[i].getShardRouting().getIndexName(); - if (!currentIndexName.equals(lastIndexName)) { - indicesSegments.put(lastIndexName, new IndexSegments(lastIndexName, Arrays.copyOfRange(shards, lastIdx, i))); - lastIdx = i; - } - if (i == shards.length - 1) { - indicesSegments.put(currentIndexName, new IndexSegments(currentIndexName, Arrays.copyOfRange(shards, lastIdx, i + 1))); + if (!shards[i].getShardRouting().getIndexName().equals(startIndexName)) { + indicesSegments.put(startIndexName, new IndexSegments(startIndexName, Arrays.copyOfRange(shards, startIndexPos, i))); + startIndexPos = i; + startIndexName = shards[startIndexPos].getShardRouting().getIndexName(); } } + // Add the last shardSegment from shards list which would have got missed in the loop above + indicesSegments.put( + shards[shards.length - 1].getShardRouting().getIndexName(), + new IndexSegments( + shards[shards.length - 1].getShardRouting().getIndexName(), + Arrays.copyOfRange(shards, startIndexPos, shards.length) + ) + ); + + this.indicesSegments = indicesSegments; return indicesSegments; } From 84a5609249ef992bae07f200964bdb7f2d2cf8cf Mon Sep 17 00:00:00 2001 From: Harsh Garg Date: Sun, 1 Sep 2024 02:30:28 +0530 Subject: [PATCH 4/6] Adding UT to assert indicesSegments Signed-off-by: Harsh Garg --- .../segments/IndicesSegmentResponse.java | 8 +--- .../segments/IndicesSegmentResponseTests.java | 48 +++++++++++++++++++ 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java b/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java index ad37887ed947a..b69d4b6f624b8 100644 --- a/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java +++ b/server/src/main/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponse.java @@ -101,13 +101,7 @@ public Map getIndices() { } } // Add the last shardSegment from shards list which would have got missed in the loop above - indicesSegments.put( - shards[shards.length - 1].getShardRouting().getIndexName(), - new IndexSegments( - shards[shards.length - 1].getShardRouting().getIndexName(), - Arrays.copyOfRange(shards, startIndexPos, shards.length) - ) - ); + indicesSegments.put(startIndexName, new IndexSegments(startIndexName, Arrays.copyOfRange(shards, startIndexPos, shards.length))); this.indicesSegments = indicesSegments; return indicesSegments; diff --git a/server/src/test/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponseTests.java b/server/src/test/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponseTests.java index b90be87e3a600..7d518966addfa 100644 --- a/server/src/test/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponseTests.java +++ b/server/src/test/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponseTests.java @@ -42,7 +42,10 @@ import org.opensearch.index.engine.Segment; import org.opensearch.test.OpenSearchTestCase; +import java.util.ArrayList; import java.util.Collections; +import java.util.List; +import java.util.Map; import static org.opensearch.common.xcontent.XContentFactory.jsonBuilder; @@ -68,4 +71,49 @@ public void testToXContentSerialiationWithSortedFields() throws Exception { response.toXContent(builder, ToXContent.EMPTY_PARAMS); } } + + public void testGetIndices() { + final int totalIndices = 5; + final int shardsPerIndex = 3; + final int segmentsPerShard = 2; + // Preparing a ShardSegments list, which will have (totalIndices * shardsPerIndex) shardSegments. + // Indices will be named -> foo1, foo2, ..., foo{totalIndices} + List shardSegmentsList = new ArrayList<>(); + for (int indexName = 0; indexName < totalIndices; indexName++) { + for (int shardId = 0; shardId < shardsPerIndex; shardId++) { + ShardRouting shardRouting = TestShardRouting.newShardRouting( + "foo" + indexName, + shardId, + "node_id", + true, + ShardRoutingState.STARTED + ); + List segmentList = new ArrayList<>(); + for (int segmentNum = 0; segmentNum < segmentsPerShard; segmentNum++) { + segmentList.add(new Segment("foo" + indexName + shardId + segmentNum)); + } + shardSegmentsList.add(new ShardSegments(shardRouting, segmentList)); + } + } + + // Prepare the IndicesSegmentResponse object and get the indicesSegments map + IndicesSegmentResponse response = new IndicesSegmentResponse( + shardSegmentsList.toArray(new ShardSegments[0]), + totalIndices * shardsPerIndex, + totalIndices * shardsPerIndex, + 0, + Collections.emptyList() + ); + Map indicesSegments = response.getIndices(); + + assertEquals(totalIndices, indicesSegments.size()); + for (Map.Entry indexSegmentEntry : indicesSegments.entrySet()) { + assertEquals(shardsPerIndex, indexSegmentEntry.getValue().getShards().size()); + for (IndexShardSegments indexShardSegment : indexSegmentEntry.getValue().getShards().values()) { + for (ShardSegments shardSegment : indexShardSegment.getShards()) { + assertEquals(segmentsPerShard, shardSegment.getSegments().size()); + } + } + } + } } From a2ed19494c6a9e07562967494299882fe6e3c96d Mon Sep 17 00:00:00 2001 From: Harsh Garg Date: Wed, 4 Sep 2024 08:57:47 +0530 Subject: [PATCH 5/6] Asserting segment's name in test Signed-off-by: Harsh Garg --- .../admin/indices/segments/IndicesSegmentResponseTests.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/server/src/test/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponseTests.java b/server/src/test/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponseTests.java index 7d518966addfa..934bd4d4d7ce8 100644 --- a/server/src/test/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponseTests.java +++ b/server/src/test/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponseTests.java @@ -43,6 +43,7 @@ import org.opensearch.test.OpenSearchTestCase; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.Map; @@ -95,6 +96,7 @@ public void testGetIndices() { shardSegmentsList.add(new ShardSegments(shardRouting, segmentList)); } } + Collections.shuffle(shardSegmentsList); // Prepare the IndicesSegmentResponse object and get the indicesSegments map IndicesSegmentResponse response = new IndicesSegmentResponse( @@ -112,6 +114,10 @@ public void testGetIndices() { for (IndexShardSegments indexShardSegment : indexSegmentEntry.getValue().getShards().values()) { for (ShardSegments shardSegment : indexShardSegment.getShards()) { assertEquals(segmentsPerShard, shardSegment.getSegments().size()); + for (int i = 0; i < segmentsPerShard; i++) { + String segmentName = indexSegmentEntry.getKey() + shardSegment.getShardRouting().getId() + i; + assertEquals(segmentName, shardSegment.getSegments().get(i).getName()); + } } } } From 2406b2320e74d2c22329e6294437e23dbcd3e68f Mon Sep 17 00:00:00 2001 From: Harsh Garg Date: Wed, 4 Sep 2024 09:16:27 +0530 Subject: [PATCH 6/6] Precommit fix Signed-off-by: Harsh Garg --- .../admin/indices/segments/IndicesSegmentResponseTests.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/test/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponseTests.java b/server/src/test/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponseTests.java index 934bd4d4d7ce8..c01335eb38afc 100644 --- a/server/src/test/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponseTests.java +++ b/server/src/test/java/org/opensearch/action/admin/indices/segments/IndicesSegmentResponseTests.java @@ -43,7 +43,6 @@ import org.opensearch.test.OpenSearchTestCase; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.Map; @@ -96,7 +95,7 @@ public void testGetIndices() { shardSegmentsList.add(new ShardSegments(shardRouting, segmentList)); } } - Collections.shuffle(shardSegmentsList); + Collections.shuffle(shardSegmentsList, random()); // Prepare the IndicesSegmentResponse object and get the indicesSegments map IndicesSegmentResponse response = new IndicesSegmentResponse(