Skip to content

Avoid extra allocations in RestGetAliasesAction #126177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Apr 7, 2025
Merged

Avoid extra allocations in RestGetAliasesAction #126177

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2d824ba
Avoid extra allocations in RestGetAliasesAction
JeremyDahlgren Apr 3, 2025
ec666bf
Update docs/changelog/126177.yaml
JeremyDahlgren Apr 3, 2025
3694b13
Merge branch 'main' into get-alias-action-reduce-alloc
JeremyDahlgren Apr 3, 2025
7b00f2c
Delete docs/changelog/126177.yaml
JeremyDahlgren Apr 3, 2025
9bddef7
Address code review comments, add unit test case
JeremyDahlgren Apr 4, 2025
b7f0490
Merge branch 'main' into get-alias-action-reduce-alloc
JeremyDahlgren Apr 4, 2025
f117e48
Use for loops instead of streams
JeremyDahlgren Apr 5, 2025
632d818
Merge branch 'main' into get-alias-action-reduce-alloc
JeremyDahlgren Apr 5, 2025
980c8b4
Address code review comments
JeremyDahlgren Apr 7, 2025
f11e4a7
Merge branch 'main' into get-alias-action-reduce-alloc
JeremyDahlgren Apr 7, 2025
0660270
Merge branch 'main' into get-alias-action-reduce-alloc
JeremyDahlgren Apr 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 60 additions & 50 deletions server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -84,59 +85,22 @@ static RestResponse buildRestResponse(
) throws Exception {
final Set<String> indicesToDisplay = new HashSet<>();
final Set<String> returnedAliasNames = new HashSet<>();
for (final Map.Entry<String, List<AliasMetadata>> cursor : responseAliasMap.entrySet()) {
for (final AliasMetadata aliasMetadata : cursor.getValue()) {
if (aliasesExplicitlyRequested) {
// only display indices that have aliases
indicesToDisplay.add(cursor.getKey());
}
returnedAliasNames.add(aliasMetadata.alias());
}
}
dataStreamAliases.entrySet()
.stream()
.flatMap(entry -> entry.getValue().stream())
.forEach(dataStreamAlias -> returnedAliasNames.add(dataStreamAlias.getName()));
if (aliasesExplicitlyRequested) {
responseAliasMap.entrySet().stream().filter(entry -> entry.getValue().isEmpty() == false).forEach(entry -> {
Copy link
Contributor

@DaveCTurner DaveCTurner Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Streams are themselves pretty expensive in terms of both allocations and general CPU usage. Could we just use a plain old-fashioned loop here?

// only display indices that have aliases
indicesToDisplay.add(entry.getKey());
entry.getValue().forEach(aliasMetadata -> returnedAliasNames.add(aliasMetadata.alias()));
});

// compute explicitly requested aliases that have are not returned in the result
final SortedSet<String> 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]);
}
}
dataStreamAliases.entrySet()
.stream()
Copy link
Contributor

@DaveCTurner DaveCTurner Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May as well destreamify this one here too.

.flatMap(entry -> entry.getValue().stream())
.forEach(dataStreamAlias -> returnedAliasNames.add(dataStreamAlias.getName()));
}

// compute explicitly requested aliases that would not be returned in the result
final var missingAliases = computeMissingAliases(requestedAliases, returnedAliasNames);

final RestStatus status;
builder.startObject();
{
Expand Down Expand Up @@ -239,4 +203,50 @@ public RestResponse buildResponse(GetAliasesResponse response, XContentBuilder b
});
}

private static SortedSet<String> computeMissingAliases(String[] requestedAliases, Set<String> returnedAliasNames) {
if (requestedAliases.length == 0) {
return Collections.emptySortedSet();
}

final var missingAliases = new TreeSet<String>();

// 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;
}
}
22 changes: 22 additions & 0 deletions .../src/test/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesActionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand Down