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

Store DataTier Preference directly on IndexMetadata #78668

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.elasticsearch.cluster.block.ClusterBlockLevel;
import org.elasticsearch.cluster.node.DiscoveryNodeFilters;
import org.elasticsearch.cluster.routing.IndexRouting;
import org.elasticsearch.cluster.routing.allocation.DataTier;
import org.elasticsearch.cluster.routing.allocation.IndexMetadataUpdater;
import org.elasticsearch.cluster.routing.allocation.decider.DiskThresholdDecider;
import org.elasticsearch.common.collect.ImmutableOpenIntMap;
Expand Down Expand Up @@ -400,6 +401,9 @@ public static APIBlock readFrom(StreamInput input) throws IOException {

private final boolean ignoreDiskWatermarks;

@Nullable // since we store null if DataTier.TIER_PREFERENCE_SETTING failed validation
private final List<String> tierPreference;

private IndexMetadata(
final Index index,
final long version,
Expand Down Expand Up @@ -429,7 +433,8 @@ private IndexMetadata(
final IndexLongFieldRange timestampRange,
final int priority,
final long creationDate,
final boolean ignoreDiskWatermarks
final boolean ignoreDiskWatermarks,
@Nullable final List<String> tierPreference
) {

this.index = index;
Expand Down Expand Up @@ -468,6 +473,7 @@ private IndexMetadata(
this.priority = priority;
this.creationDate = creationDate;
this.ignoreDiskWatermarks = ignoreDiskWatermarks;
this.tierPreference = tierPreference;
assert numberOfShards * routingFactor == routingNumShards : routingNumShards + " must be a multiple of " + numberOfShards;
}

Expand Down Expand Up @@ -574,6 +580,15 @@ public ImmutableOpenMap<String, AliasMetadata> getAliases() {
return this.aliases;
}

public List<String> getTierPreference() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a simple test that getTierPreference delivers the right output?

Copy link
Member Author

Choose a reason for hiding this comment

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

++ added both tests

if (tierPreference == null) {
final List<String> parsed = DataTier.parseTierList(DataTier.TIER_PREFERENCE_SETTING.get(settings));
assert false : "the setting parsing should always throw if we didn't store a tier preference when building this instance";
return parsed;
}
return tierPreference;
}

/**
* Return the concrete mapping for this index or {@code null} if this index has no mappings at all.
*/
Expand Down Expand Up @@ -1311,6 +1326,17 @@ public IndexMetadata build() {

final String uuid = settings.get(SETTING_INDEX_UUID, INDEX_UUID_NA_VALUE);

List<String> tierPreference;
try {
tierPreference = DataTier.parseTierList(DataTier.TIER_PREFERENCE_SETTING.get(settings));
} catch (Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this has to be an IllegalArgumentException so maybe we can catch that instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically yes, but we also call some other settings. I don't know if we want to bank on no changes to that?
I opted to be careful here mainly because this is a BwC thing to begin with and we could inadvertently introduce a bug like say some out of bounds exception with some specific old version of the setting and I wanted to avoid that. (we had a few similar BwC cases over the years that are hard to catch in tests up front)

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, can we then assert that it is an IllegalArgumentException instead? Would like to avoid hiding some other exception from tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

++ aded

assert e instanceof IllegalArgumentException : e;
// BwC hack: the setting failed validation but it will be fixed in
// #IndexMetadataVerifier#convertSharedCacheTierPreference(IndexMetadata)} later so we just store a null
// to be able to build a temporary instance
tierPreference = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a test that we can build an IndexMetadata object based on a settings object with an illegal _tier_preference?

}

return new IndexMetadata(
new Index(index, uuid),
version,
Expand Down Expand Up @@ -1340,7 +1366,8 @@ public IndexMetadata build() {
timestampRange,
IndexMetadata.INDEX_PRIORITY_SETTING.get(settings),
settings.getAsLong(SETTING_CREATION_DATE, -1L),
DiskThresholdDecider.SETTING_IGNORE_DISK_WATERMARKS.get(settings)
DiskThresholdDecider.SETTING_IGNORE_DISK_WATERMARKS.get(settings),
tierPreference
);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,33 +1,38 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

package org.elasticsearch.xpack.core;
package org.elasticsearch.cluster.routing.allocation;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.elasticsearch.cluster.metadata.IndexMetadata;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.cluster.node.DiscoveryNodeRole;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.IndexModule;
import org.elasticsearch.index.shard.IndexSettingProvider;
import org.elasticsearch.xpack.cluster.routing.allocation.DataTierAllocationDecider;
import org.elasticsearch.snapshots.SearchableSnapshotsSettings;

import java.util.Collection;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.Function;
import java.util.stream.Collectors;

/**
* The {@code DataTier} class encapsulates the formalization of the "content",
* "hot", "warm", and "cold" tiers as node roles. In contains the
* roles themselves as well as helpers for validation and determining if a node
* has a tier configured.
*
* Related:
* {@link org.elasticsearch.xpack.cluster.routing.allocation.DataTierAllocationDecider}
*/
public class DataTier {

Expand All @@ -39,6 +44,17 @@ public class DataTier {

public static final Set<String> ALL_DATA_TIERS = Set.of(DATA_CONTENT, DATA_HOT, DATA_WARM, DATA_COLD, DATA_FROZEN);

public static final String TIER_PREFERENCE = "index.routing.allocation.include._tier_preference";

public static final Setting<String> TIER_PREFERENCE_SETTING = new Setting<>(
new Setting.SimpleKey(TIER_PREFERENCE),
DataTierSettingValidator::getDefaultTierPreference,
Function.identity(),
new DataTierSettingValidator(),
Setting.Property.Dynamic,
Setting.Property.IndexScope
);

static {
for (String tier : ALL_DATA_TIERS) {
assert tier.equals(DATA_FROZEN) || tier.contains(DATA_FROZEN) == false
Expand All @@ -59,8 +75,8 @@ public static boolean validTierName(String tierName) {

/**
* Based on the provided target tier it will return a comma separated list of preferred tiers.
* ie. if `data_cold` is the target tier, it will return `data_cold,data_warm,data_hot`
* This is usually used in conjunction with {@link DataTierAllocationDecider#TIER_PREFERENCE_SETTING}
Copy link
Contributor

Choose a reason for hiding this comment

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

Think we can keep this, the setting is still in scope here.

Copy link
Member Author

Choose a reason for hiding this comment

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

++ brought this back

Copy link
Contributor

Choose a reason for hiding this comment

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

u sure? I don't see it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Now :) Sorry accidentally put this on the top level class.

* ie. if `data_cold` is the target tier, it will return `data_cold,data_warm,data_hot`.
* This is usually used in conjunction with {@link #TIER_PREFERENCE_SETTING}.
*/
public static String getPreferredTiersConfiguration(String targetTier) {
int indexOfTargetTier = ORDERED_FROZEN_TO_HOT_TIERS.indexOf(targetTier);
Expand Down Expand Up @@ -115,6 +131,15 @@ public static boolean isFrozenNode(final Set<DiscoveryNodeRole> roles) {
return roles.contains(DiscoveryNodeRole.DATA_FROZEN_NODE_ROLE) || roles.contains(DiscoveryNodeRole.DATA_ROLE);
}

public static List<String> parseTierList(String tiers) {
if (Strings.hasText(tiers) == false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Bit weird that we treat all-whitespace as empty but otherwise we're whitespace-sensitive. (Acking that this is how it was before too, no action required)

// avoid parsing overhead in the null/empty string case
return List.of();
} else {
return List.of(tiers.split(","));
}
}

/**
* This setting provider injects the setting allocating all newly created indices with
* {@code index.routing.allocation.include._tier_preference: "data_hot"} for a data stream index
Expand All @@ -128,9 +153,9 @@ public static class DefaultHotAllocationSettingProvider implements IndexSettingP
@Override
public Settings getAdditionalIndexSettings(String indexName, boolean isDataStreamIndex, Settings indexSettings) {
Set<String> settings = indexSettings.keySet();
if (settings.contains(DataTierAllocationDecider.TIER_PREFERENCE)) {
if (settings.contains(TIER_PREFERENCE)) {
// just a marker -- this null value will be removed or overridden by the template/request settings
return Settings.builder().putNull(DataTierAllocationDecider.TIER_PREFERENCE).build();
return Settings.builder().putNull(TIER_PREFERENCE).build();
} else if (settings.stream().anyMatch(s -> s.startsWith(IndexMetadata.INDEX_ROUTING_REQUIRE_GROUP_PREFIX + ".")) ||
settings.stream().anyMatch(s -> s.startsWith(IndexMetadata.INDEX_ROUTING_EXCLUDE_GROUP_PREFIX + ".")) ||
settings.stream().anyMatch(s -> s.startsWith(IndexMetadata.INDEX_ROUTING_INCLUDE_GROUP_PREFIX + "."))) {
Expand All @@ -142,11 +167,60 @@ public Settings getAdditionalIndexSettings(String indexName, boolean isDataStrea
// tier if the index is part of a data stream, the "content"
// tier if it is not.
if (isDataStreamIndex) {
return Settings.builder().put(DataTierAllocationDecider.TIER_PREFERENCE, DATA_HOT).build();
return Settings.builder().put(TIER_PREFERENCE, DATA_HOT).build();
} else {
return Settings.builder().put(TIER_PREFERENCE, DATA_CONTENT).build();
}
}
}
}

private static final class DataTierSettingValidator implements Setting.Validator<String> {

private static final Collection<Setting<?>> dependencies = List.of(
IndexModule.INDEX_STORE_TYPE_SETTING,
SearchableSnapshotsSettings.SNAPSHOT_PARTIAL_SETTING
);

public static String getDefaultTierPreference(Settings settings) {
if (SearchableSnapshotsSettings.isPartialSearchableSnapshotIndex(settings)) {
return DATA_FROZEN;
} else {
return "";
}
}

@Override
public void validate(String value) {
if (Strings.hasText(value)) {
for (String s : parseTierList(value)) {
if (validTierName(s) == false) {
throw new IllegalArgumentException(
"invalid tier names found in [" + value + "] allowed values are " + ALL_DATA_TIERS);
}
}
}
}

@Override
public void validate(String value, Map<Setting<?>, Object> settings, boolean exists) {
if (exists && value != null) {
if (SearchableSnapshotsSettings.isPartialSearchableSnapshotIndex(settings)) {
if (value.equals(DATA_FROZEN) == false) {
throw new IllegalArgumentException("only the [" + DATA_FROZEN +
"] tier preference may be used for partial searchable snapshots (got: [" + value + "])");
}
} else {
return Settings.builder().put(DataTierAllocationDecider.TIER_PREFERENCE, DATA_CONTENT).build();
if (value.contains(DATA_FROZEN)) {
throw new IllegalArgumentException("[" + DATA_FROZEN + "] tier can only be used for partial searchable snapshots");
}
}
}
}

@Override
public Iterator<Setting<?>> settings() {
return dependencies.iterator();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,25 @@

package org.elasticsearch.snapshots;

import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;

import java.util.Map;

import static org.elasticsearch.index.IndexModule.INDEX_STORE_TYPE_SETTING;

public final class SearchableSnapshotsSettings {

public static final String SEARCHABLE_SNAPSHOT_STORE_TYPE = "snapshot";
public static final String SEARCHABLE_SNAPSHOT_PARTIAL_SETTING_KEY = "index.store.snapshot.partial";

public static final Setting<Boolean> SNAPSHOT_PARTIAL_SETTING = Setting.boolSetting(
SEARCHABLE_SNAPSHOT_PARTIAL_SETTING_KEY,
false,
Setting.Property.IndexScope,
Setting.Property.PrivateIndex,
Setting.Property.NotCopyableOnResize
);
public static final String SEARCHABLE_SNAPSHOTS_REPOSITORY_NAME_SETTING_KEY = "index.store.snapshot.repository_name";
public static final String SEARCHABLE_SNAPSHOTS_REPOSITORY_UUID_SETTING_KEY = "index.store.snapshot.repository_uuid";
public static final String SEARCHABLE_SNAPSHOTS_SNAPSHOT_NAME_SETTING_KEY = "index.store.snapshot.snapshot_name";
Expand All @@ -31,4 +42,16 @@ public static boolean isSearchableSnapshotStore(Settings indexSettings) {
public static boolean isPartialSearchableSnapshotIndex(Settings indexSettings) {
return isSearchableSnapshotStore(indexSettings) && indexSettings.getAsBoolean(SEARCHABLE_SNAPSHOT_PARTIAL_SETTING_KEY, false);
}

/**
* Based on a map from setting to value, do the settings represent a partial searchable snapshot index?
*
* Both index.store.type and index.store.snapshot.partial must be supplied.
*/
public static boolean isPartialSearchableSnapshotIndex(Map<Setting<?>, Object> indexSettings) {
assert indexSettings.containsKey(INDEX_STORE_TYPE_SETTING) : "must include store type in map";
assert indexSettings.get(SNAPSHOT_PARTIAL_SETTING) != null : "partial setting must be non-null in map (has default value)";
return SEARCHABLE_SNAPSHOT_STORE_TYPE.equals(indexSettings.get(INDEX_STORE_TYPE_SETTING))
&& (boolean) indexSettings.get(SNAPSHOT_PARTIAL_SETTING);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.elasticsearch.action.admin.indices.rollover.MaxPrimaryShardSizeCondition;
import org.elasticsearch.action.admin.indices.rollover.MaxSizeCondition;
import org.elasticsearch.action.admin.indices.rollover.RolloverInfo;
import org.elasticsearch.cluster.routing.allocation.DataTier;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.collect.ImmutableOpenMap;
Expand Down Expand Up @@ -388,4 +389,26 @@ public void testIsHidden() {
assertTrue(indexMetadata.isHidden()); // preserved if settings unchanged
}

public void testGetTierPreference() {
final Settings indexSettings = indexSettingsWithDataTier("data_warm,data_cold");
final IndexMetadata indexMetadata = IndexMetadata.builder("myindex").settings(indexSettings).build();
assertThat(indexMetadata.getTierPreference(), is(DataTier.parseTierList(DataTier.TIER_PREFERENCE_SETTING.get(indexSettings))));
assertThat(indexMetadata.getTierPreference(), is(List.of(DataTier.DATA_WARM, DataTier.DATA_COLD)));

}

public void testBuildsWithBrokenTierPreference() {
final Settings indexSettings = indexSettingsWithDataTier("broken_tier");
final IndexMetadata indexMetadata = IndexMetadata.builder("myindex").settings(indexSettings).build();
expectThrows(IllegalArgumentException.class, indexMetadata::getTierPreference);
}

private static Settings indexSettingsWithDataTier(String dataTier) {
return Settings.builder()
.put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT)
.put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
.put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0)
.put(DataTier.TIER_PREFERENCE, dataTier)
.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import org.elasticsearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.cluster.node.DiscoveryNodeRole;
import org.elasticsearch.cluster.routing.allocation.DataTier;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.ByteSizeUnit;
Expand All @@ -22,7 +23,6 @@
import org.elasticsearch.xpack.autoscaling.action.PutAutoscalingPolicyAction;
import org.elasticsearch.xpack.autoscaling.capacity.AutoscalingCapacity;
import org.elasticsearch.xpack.autoscaling.shards.LocalStateAutoscalingAndSearchableSnapshots;
import org.elasticsearch.xpack.core.DataTier;
import org.elasticsearch.xpack.core.searchablesnapshots.MountSearchableSnapshotAction;
import org.elasticsearch.xpack.core.searchablesnapshots.MountSearchableSnapshotRequest;
import org.elasticsearch.xpack.searchablesnapshots.cache.shared.FrozenCacheService;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,14 @@
import org.elasticsearch.cluster.InternalClusterInfoService;
import org.elasticsearch.cluster.metadata.IndexMetadata;
import org.elasticsearch.cluster.node.DiscoveryNodeRole;
import org.elasticsearch.cluster.routing.allocation.DataTier;
import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.node.Node;
import org.elasticsearch.test.ESIntegTestCase;
import org.elasticsearch.test.NodeRoles;
import org.elasticsearch.xpack.autoscaling.action.GetAutoscalingCapacityAction;
import org.elasticsearch.xpack.autoscaling.action.PutAutoscalingPolicyAction;
import org.elasticsearch.xpack.cluster.routing.allocation.DataTierAllocationDecider;
import org.elasticsearch.xpack.core.DataTier;
import org.hamcrest.Matchers;

import java.util.Arrays;
Expand Down Expand Up @@ -118,7 +117,7 @@ private void testScaleFromEmptyWarm(boolean allocatable) throws Exception {
.put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0)
.put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 6)
.put(INDEX_STORE_STATS_REFRESH_INTERVAL_SETTING.getKey(), "0ms")
.put(DataTierAllocationDecider.TIER_PREFERENCE, allocatable ? "data_hot" : "data_content")
.put(DataTier.TIER_PREFERENCE, allocatable ? "data_hot" : "data_content")
.build()
).setWaitForActiveShards(allocatable ? ActiveShardCount.DEFAULT : ActiveShardCount.NONE)
);
Expand All @@ -131,9 +130,7 @@ private void testScaleFromEmptyWarm(boolean allocatable) throws Exception {
client().admin()
.indices()
.updateSettings(
new UpdateSettingsRequest(indexName).settings(
Settings.builder().put(DataTierAllocationDecider.TIER_PREFERENCE, "data_warm,data_hot")
)
new UpdateSettingsRequest(indexName).settings(Settings.builder().put(DataTier.TIER_PREFERENCE, "data_warm,data_hot"))
)
.actionGet()
);
Expand Down
Loading