Skip to content
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.

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

Optimize DataTierAllocationDecider Further #78235

Conversation

original-brownbear
Copy link
Member

This decider is still by far the most expensive component of reroute operations on very large clusters.
Optimizing away some more costly allocations and slow stream loops.

This decider is still by far the most expensive component of reroute operations.
Optimizing away some more costly allocations and slow stream loops.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

LGTM, except for the regexp use.

@@ -90,8 +91,7 @@ public void validate(String value, Map<Setting<?>, Object> settings, boolean exi
"] tier preference may be used for partial searchable snapshots (got: [" + value + "])");
}
} else {
String[] split = value.split(",");
if (Arrays.stream(split).anyMatch(DATA_FROZEN::equals)) {
if (FROZEN_TIER_PATTERN.matcher(value).find()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not really sure about this bit, might run faster, might not?

Perhaps we can add a test validating that just doing value.indexOf(DATA_FROZEN) is safe, i.e., that there are no other tiers starting with DATA_FROZEN?

Copy link
Member Author

Choose a reason for hiding this comment

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

Jup see d62ad36 that should be much faster and is more fun to read as well :)

@original-brownbear
Copy link
Member Author

Thanks Henning!

@original-brownbear original-brownbear merged commit 86b8063 into elastic:master Sep 23, 2021
@original-brownbear original-brownbear deleted the more-fixes-data-tier-alloc-decider branch September 23, 2021 13:20
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Sep 23, 2021
This decider is still by far the most expensive component of reroute operations.
Optimizing away some more costly allocations and slow stream loops.
original-brownbear added a commit that referenced this pull request Sep 23, 2021
This decider is still by far the most expensive component of reroute operations.
Optimizing away some more costly allocations and slow stream loops.
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

I know this is already merged, but I left a comment about behavior that this changes

@dakrone
Copy link
Member

dakrone commented Sep 23, 2021

My other concern about this is that without any code comments about why these changes were made, we run the risk of accidentally undoing these changes in a subsequent PR. (something like "oh, I like streams, I'll use those rather than this old for loop")

@original-brownbear original-brownbear restored the more-fixes-data-tier-alloc-decider branch April 18, 2023 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants