Skip to content

ILM: migrate action configures the _tier_preference setting #62829

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

Merged
merged 10 commits into from
Sep 24, 2020

Conversation

andreidan
Copy link
Contributor

The migrate action will now configure the
index.routing.allocation.include._tier_preference setting to the corresponding
tiers. For the HOT phase it will configure data_hot, for the WARM phase it will
configure data_warm,data_hot and for the COLD phase
data_cold,data_warm,data_cold.

Relates to #60848

The `migrate` action will now configure the
`index.routing.allocation.include._tier_preference` setting to the corresponding
tiers. For the HOT phase it will configure `data_hot`, for the WARM phase it will
configure `data_warm,data_hot` and for the COLD phase
`data_cold,data_warm,data_cold`.
@andreidan andreidan added :Data Management/ILM+SLM Index and Snapshot lifecycle management v8.0.0 v7.10.0 labels Sep 23, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/ILM+SLM)

@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Sep 23, 2020
@andreidan andreidan requested a review from dakrone September 23, 2020 14:03
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.

Thanks Andrei, I left some comments below but they're pretty minor, otherwise this LGTM

logger.debug("[{}] lifecycle action for index [{}] cannot make progress because not all shards are active",
getKey().getAction(), index.getName());
} else {
logger.debug("[{}] migration of index [{}] to the [{}] tier cannot progress, as not all shards are active",
getKey().getAction(), index.getName(), destinationTier);
if (Strings.hasText(availableDestinationTier)) {
Copy link
Member

Choose a reason for hiding this comment

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

this becomes the equivalent of a null check with the .orElse("") above, maybe we can just leave it in an Optional<String> above and do availableDestinationTier.isPresent(), which I think is a bit clearer than checking for text (especially since we don't care what the text is)

Comment on lines 106 to 107
statusMessage = String.format(Locale.ROOT, "[%s] lifecycle action [%s] waiting for [%s] shards to be moved to the [%s] " +
"tier", index.getName(), getKey().getAction(), allocationPendingAllShards, availableDestinationTier);
Copy link
Member

Choose a reason for hiding this comment

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

I think in this message we should include the original preference setting, or else I think users will be confused why their cold phase index is waiting to move to the data_warm tier (if they see preference [data_cold,data_warm,data_hot] in the message I hope it will be clearer to them)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point, updating the message to include the preference. Thanks Lee

throw new IllegalArgumentException("invalid data tier [" + targetTier + "]");
}
String preferredTiers = COLD_TO_HOT_TIERS.stream().skip(indexOfTargetTier).collect(Collectors.joining(","));
if (Strings.hasText(preferredTiers) == false) {
Copy link
Member

@dakrone dakrone Sep 23, 2020

Choose a reason for hiding this comment

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

If we keep this, we could reverse the double negative with

Suggested change
if (Strings.hasText(preferredTiers) == false) {
if (Strings.isEmpty(preferredTiers)) {

But what I'm trying to understand is how this would actually be empty, even in the hot phase the stream skip above would at least return "data_hot", and we check that the index is not -1 above, so I'm not sure how it actually gets into this state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is my paranoia, you're right, I'll drop it

andreidan and others added 8 commits September 23, 2020 16:32
Co-authored-by: Lee Hinman <dakrone@users.noreply.github.com>
Co-authored-by: Lee Hinman <dakrone@users.noreply.github.com>
Co-authored-by: Lee Hinman <dakrone@users.noreply.github.com>
@andreidan
Copy link
Contributor Author

@elasticmachine update branch

@andreidan andreidan merged commit 9dbf0e6 into elastic:master Sep 24, 2020
andreidan added a commit to andreidan/elasticsearch that referenced this pull request Sep 24, 2020
…62829)

The `migrate` action will now configure the
`index.routing.allocation.include._tier_preference` setting to the corresponding
tiers. For the HOT phase it will configure `data_hot`, for the WARM phase it will
configure `data_warm,data_hot` and for the COLD phase
`data_cold,data_warm,data_cold`.

(cherry picked from commit 9dbf0e6)
Signed-off-by: Andrei Dan <andrei.dan@elastic.co>
andreidan added a commit that referenced this pull request Sep 24, 2020
…2829) (#62860)

The `migrate` action will now configure the
`index.routing.allocation.include._tier_preference` setting to the corresponding
tiers. For the HOT phase it will configure `data_hot`, for the WARM phase it will
configure `data_warm,data_hot` and for the COLD phase
`data_cold,data_warm,data_cold`.

(cherry picked from commit 9dbf0e6)
Signed-off-by: Andrei Dan <andrei.dan@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/ILM+SLM Index and Snapshot lifecycle management Team:Data Management Meta label for data/management team v7.10.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants