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

Changes from 4 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
29 changes: 19 additions & 10 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 @@ -84,19 +84,28 @@ 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());
if (aliasesExplicitlyRequested || requestedAliases.length > 0) {
Copy link
Contributor

@albertzaharovits albertzaharovits Apr 3, 2025

Choose a reason for hiding this comment

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

I've peered into the original code...
Does aliasesExplicitlyRequested have any influence? I think we can remove it (consider it either true or false in the original code).

Copy link
Contributor Author

@JeremyDahlgren JeremyDahlgren Apr 4, 2025

Choose a reason for hiding this comment

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

Yes, it can be true even when requestedAliases.length == 0 in the case _all is used for the name param. In prepareRequest() it uses request.paramAsStringArrayOrEmptyIfAll("name"). I originally wanted to eliminate aliasesExplicitlyRequested, but some tests broke even when the RestGetAliasesActionTests passed. An example was:

ClientYamlTestSuiteIT > test {yaml=indices.get_alias/10_basic/Get aliases via /_alias/_all} FAILED
    java.lang.AssertionError: Failure at [indices.get_alias/10_basic:70]: field [test_index_3] has a true value but it shouldn't

I added a new unit test case in RestGetAliasesActionTests that uses _all to cover this scenario.

I also refactored to simplify the code and just use aliasesExplicitlyRequested. Please let me know how this looks to you.

for (final Map.Entry<String, List<AliasMetadata>> 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()
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 have are not returned in the result
final SortedSet<String> missingAliases = new TreeSet<>();
Copy link
Contributor

@albertzaharovits albertzaharovits Apr 3, 2025

Choose a reason for hiding this comment

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

can we move the code that computes the missingAliases into a separate function.

Expand Down