From 2d824bafc3b7e7377d9d225c0fd249685a490055 Mon Sep 17 00:00:00 2001 From: Jeremy Dahlgren Date: Wed, 2 Apr 2025 22:10:47 -0400 Subject: [PATCH 1/6] Avoid extra allocations in RestGetAliasesAction When no explicit aliases are provided in the call there is no need to collect the index names or aliases into HashSets if they won't be used. Also fixed where the index name was being added for each loop of the alias list. --- .../admin/indices/RestGetAliasesAction.java | 29 ++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java index 841d7979862dc..1d5bcbe30d489 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java @@ -84,19 +84,28 @@ static RestResponse buildRestResponse( ) throws Exception { final Set indicesToDisplay = new HashSet<>(); final Set returnedAliasNames = new HashSet<>(); - for (final Map.Entry> cursor : responseAliasMap.entrySet()) { - for (final AliasMetadata aliasMetadata : cursor.getValue()) { - if (aliasesExplicitlyRequested) { - // only display indices that have aliases - indicesToDisplay.add(cursor.getKey()); + if (aliasesExplicitlyRequested || requestedAliases.length > 0) { + for (final Map.Entry> cursor : responseAliasMap.entrySet()) { + final var aliases = cursor.getValue(); + if (aliases.isEmpty() == false) { + if (aliasesExplicitlyRequested) { + // only display indices that have aliases + indicesToDisplay.add(cursor.getKey()); + } + if (requestedAliases.length > 0) { + for (final AliasMetadata aliasMetadata : aliases) { + returnedAliasNames.add(aliasMetadata.alias()); + } + } } - returnedAliasNames.add(aliasMetadata.alias()); } } - dataStreamAliases.entrySet() - .stream() - .flatMap(entry -> entry.getValue().stream()) - .forEach(dataStreamAlias -> returnedAliasNames.add(dataStreamAlias.getName())); + if (requestedAliases.length > 0) { + dataStreamAliases.entrySet() + .stream() + .flatMap(entry -> entry.getValue().stream()) + .forEach(dataStreamAlias -> returnedAliasNames.add(dataStreamAlias.getName())); + } // compute explicitly requested aliases that have are not returned in the result final SortedSet missingAliases = new TreeSet<>(); From ec666bf07dbe4e1f1c84c6b948e8d4d43c52fed1 Mon Sep 17 00:00:00 2001 From: Jeremy Dahlgren Date: Wed, 2 Apr 2025 22:11:00 -0400 Subject: [PATCH 2/6] Update docs/changelog/126177.yaml --- docs/changelog/126177.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/126177.yaml diff --git a/docs/changelog/126177.yaml b/docs/changelog/126177.yaml new file mode 100644 index 0000000000000..473675bc98e4d --- /dev/null +++ b/docs/changelog/126177.yaml @@ -0,0 +1,5 @@ +pr: 126177 +summary: Avoid extra allocations in `RestGetAliasesAction` +area: Distributed +type: enhancement +issues: [] From 7b00f2ce4854e24e8d356abe6d41f289582d7262 Mon Sep 17 00:00:00 2001 From: Jeremy Dahlgren Date: Thu, 3 Apr 2025 12:57:27 -0400 Subject: [PATCH 3/6] Delete docs/changelog/126177.yaml --- docs/changelog/126177.yaml | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 docs/changelog/126177.yaml diff --git a/docs/changelog/126177.yaml b/docs/changelog/126177.yaml deleted file mode 100644 index 473675bc98e4d..0000000000000 --- a/docs/changelog/126177.yaml +++ /dev/null @@ -1,5 +0,0 @@ -pr: 126177 -summary: Avoid extra allocations in `RestGetAliasesAction` -area: Distributed -type: enhancement -issues: [] From 9bddef7e61f35de7764b81f77265b724c7f227c8 Mon Sep 17 00:00:00 2001 From: Jeremy Dahlgren Date: Fri, 4 Apr 2025 15:43:08 -0400 Subject: [PATCH 4/6] Address code review comments, add unit test case --- .../admin/indices/RestGetAliasesAction.java | 111 +++++++++--------- .../indices/RestGetAliasesActionTests.java | 22 ++++ 2 files changed, 78 insertions(+), 55 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java index 1d5bcbe30d489..3abe352a535e5 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java @@ -36,6 +36,7 @@ import org.elasticsearch.xcontent.XContentBuilder; import java.io.IOException; +import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Locale; @@ -84,67 +85,21 @@ static RestResponse buildRestResponse( ) throws Exception { final Set indicesToDisplay = new HashSet<>(); final Set returnedAliasNames = new HashSet<>(); - if (aliasesExplicitlyRequested || requestedAliases.length > 0) { - for (final Map.Entry> cursor : responseAliasMap.entrySet()) { - final var aliases = cursor.getValue(); - if (aliases.isEmpty() == false) { - if (aliasesExplicitlyRequested) { - // only display indices that have aliases - indicesToDisplay.add(cursor.getKey()); - } - if (requestedAliases.length > 0) { - for (final AliasMetadata aliasMetadata : aliases) { - returnedAliasNames.add(aliasMetadata.alias()); - } - } - } - } - } - if (requestedAliases.length > 0) { + if (aliasesExplicitlyRequested) { + responseAliasMap.entrySet().stream().filter(entry -> entry.getValue().isEmpty() == false).forEach(entry -> { + // only display indices that have aliases + indicesToDisplay.add(entry.getKey()); + entry.getValue().forEach(aliasMetadata -> returnedAliasNames.add(aliasMetadata.alias())); + }); + dataStreamAliases.entrySet() .stream() .flatMap(entry -> entry.getValue().stream()) .forEach(dataStreamAlias -> returnedAliasNames.add(dataStreamAlias.getName())); } - // compute explicitly requested aliases that have are not returned in the result - final SortedSet missingAliases = new TreeSet<>(); - // first wildcard index, leading "-" as an alias name after this index means - // that it is an exclusion - int firstWildcardIndex = requestedAliases.length; - for (int i = 0; i < requestedAliases.length; i++) { - if (Regex.isSimpleMatchPattern(requestedAliases[i])) { - firstWildcardIndex = i; - break; - } - } - for (int i = 0; i < requestedAliases.length; i++) { - if (Metadata.ALL.equals(requestedAliases[i]) - || Regex.isSimpleMatchPattern(requestedAliases[i]) - || (i > firstWildcardIndex && requestedAliases[i].charAt(0) == '-')) { - // only explicitly requested aliases will be called out as missing (404) - continue; - } - // check if aliases[i] is subsequently excluded - int j = Math.max(i + 1, firstWildcardIndex); - for (; j < requestedAliases.length; j++) { - if (requestedAliases[j].charAt(0) == '-') { - // this is an exclude pattern - if (Regex.simpleMatch(requestedAliases[j].substring(1), requestedAliases[i]) - || Metadata.ALL.equals(requestedAliases[j].substring(1))) { - // aliases[i] is excluded by aliases[j] - break; - } - } - } - if (j == requestedAliases.length) { - // explicitly requested aliases[i] is not excluded by any subsequent "-" wildcard in expression - if (false == returnedAliasNames.contains(requestedAliases[i])) { - // aliases[i] is not in the result set - missingAliases.add(requestedAliases[i]); - } - } - } + // compute explicitly requested aliases that would not be returned in the result + final var missingAliases = computeMissingAliases(requestedAliases, returnedAliasNames); final RestStatus status; builder.startObject(); @@ -248,4 +203,50 @@ public RestResponse buildResponse(GetAliasesResponse response, XContentBuilder b }); } + private static SortedSet computeMissingAliases(String[] requestedAliases, Set returnedAliasNames) { + if (requestedAliases.length == 0) { + return Collections.emptySortedSet(); + } + + final var missingAliases = new TreeSet(); + + // first wildcard index, leading "-" as an alias name after this index means + // that it is an exclusion + int firstWildcardIndex = requestedAliases.length; + for (int i = 0; i < requestedAliases.length; i++) { + if (Regex.isSimpleMatchPattern(requestedAliases[i])) { + firstWildcardIndex = i; + break; + } + } + for (int i = 0; i < requestedAliases.length; i++) { + if (Metadata.ALL.equals(requestedAliases[i]) + || Regex.isSimpleMatchPattern(requestedAliases[i]) + || (i > firstWildcardIndex && requestedAliases[i].charAt(0) == '-')) { + // only explicitly requested aliases will be called out as missing (404) + continue; + } + // check if aliases[i] is subsequently excluded + int j = Math.max(i + 1, firstWildcardIndex); + for (; j < requestedAliases.length; j++) { + if (requestedAliases[j].charAt(0) == '-') { + // this is an exclude pattern + if (Regex.simpleMatch(requestedAliases[j].substring(1), requestedAliases[i]) + || Metadata.ALL.equals(requestedAliases[j].substring(1))) { + // aliases[i] is excluded by aliases[j] + break; + } + } + } + if (j == requestedAliases.length) { + // explicitly requested aliases[i] is not excluded by any subsequent "-" wildcard in expression + if (false == returnedAliasNames.contains(requestedAliases[i])) { + // aliases[i] is not in the result set + missingAliases.add(requestedAliases[i]); + } + } + } + + return missingAliases; + } } diff --git a/server/src/test/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesActionTests.java b/server/src/test/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesActionTests.java index 57499618af28d..928647dcaba06 100644 --- a/server/src/test/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesActionTests.java +++ b/server/src/test/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesActionTests.java @@ -12,6 +12,7 @@ import org.elasticsearch.cluster.metadata.AliasMetadata; import org.elasticsearch.rest.RestResponse; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.rest.FakeRestRequest; import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentFactory; import org.elasticsearch.xcontent.XContentType; @@ -51,6 +52,27 @@ public void testBareRequest() throws Exception { assertThat(restResponse.content().utf8ToString(), equalTo("{\"index\":{\"aliases\":{\"foo\":{},\"foobar\":{}}}}")); } + public void testNameParamWithAllValue() throws Exception { + final XContentBuilder xContentBuilder = XContentFactory.contentBuilder(XContentType.JSON); + final var req = new FakeRestRequest.Builder(xContentRegistry()).withParams(Map.of("name", "_all")).build(); + final RestResponse restResponse = RestGetAliasesAction.buildRestResponse( + req.hasParam("name"), + req.paramAsStringArrayOrEmptyIfAll("name"), + Map.of( + "index", + Arrays.asList(AliasMetadata.builder("foo").build(), AliasMetadata.builder("foobar").build()), + "index2", + List.of() + ), + Map.of(), + xContentBuilder + ); + assertThat(restResponse.status(), equalTo(OK)); + assertThat(restResponse.contentType(), equalTo("application/json")); + // Verify we don't get "index2" since it has no aliases. + assertThat(restResponse.content().utf8ToString(), equalTo("{\"index\":{\"aliases\":{\"foo\":{},\"foobar\":{}}}}")); + } + public void testSimpleAliasWildcardMatchingNothing() throws Exception { final XContentBuilder xContentBuilder = XContentFactory.contentBuilder(XContentType.JSON); final RestResponse restResponse = RestGetAliasesAction.buildRestResponse( From f117e48a17a70540f96d738b7b4ae6919f8bdfee Mon Sep 17 00:00:00 2001 From: Jeremy Dahlgren Date: Sat, 5 Apr 2025 08:37:20 -0400 Subject: [PATCH 5/6] Use for loops instead of streams --- .../admin/indices/RestGetAliasesAction.java | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java index 3abe352a535e5..d43a768e7f2ee 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java @@ -86,16 +86,22 @@ static RestResponse buildRestResponse( final Set indicesToDisplay = new HashSet<>(); final Set returnedAliasNames = new HashSet<>(); if (aliasesExplicitlyRequested) { - responseAliasMap.entrySet().stream().filter(entry -> entry.getValue().isEmpty() == false).forEach(entry -> { - // only display indices that have aliases - indicesToDisplay.add(entry.getKey()); - entry.getValue().forEach(aliasMetadata -> returnedAliasNames.add(aliasMetadata.alias())); - }); + for (final Map.Entry> cursor : responseAliasMap.entrySet()) { + final var aliases = cursor.getValue(); + if (aliases.isEmpty() == false) { + // only display indices that have aliases + indicesToDisplay.add(cursor.getKey()); + for (final AliasMetadata aliasMetadata : cursor.getValue()) { + returnedAliasNames.add(aliasMetadata.alias()); + } + } + } - dataStreamAliases.entrySet() - .stream() - .flatMap(entry -> entry.getValue().stream()) - .forEach(dataStreamAlias -> returnedAliasNames.add(dataStreamAlias.getName())); + for (final Map.Entry> entry : dataStreamAliases.entrySet()) { + for (final DataStreamAlias dataStreamAlias : entry.getValue()) { + returnedAliasNames.add(dataStreamAlias.getName()); + } + } } // compute explicitly requested aliases that would not be returned in the result From 980c8b4fda2f938bca6f6958fd7e0aef2b1b6913 Mon Sep 17 00:00:00 2001 From: Jeremy Dahlgren Date: Mon, 7 Apr 2025 08:52:30 -0400 Subject: [PATCH 6/6] Address code review comments --- .../rest/action/admin/indices/RestGetAliasesAction.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java index d43a768e7f2ee..7f0ded9de54e7 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java @@ -91,14 +91,14 @@ static RestResponse buildRestResponse( if (aliases.isEmpty() == false) { // only display indices that have aliases indicesToDisplay.add(cursor.getKey()); - for (final AliasMetadata aliasMetadata : cursor.getValue()) { + for (final AliasMetadata aliasMetadata : aliases) { returnedAliasNames.add(aliasMetadata.alias()); } } } - for (final Map.Entry> entry : dataStreamAliases.entrySet()) { - for (final DataStreamAlias dataStreamAlias : entry.getValue()) { + for (final List dataStreamAliasList : dataStreamAliases.values()) { + for (final DataStreamAlias dataStreamAlias : dataStreamAliasList) { returnedAliasNames.add(dataStreamAlias.getName()); } }