-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Make searchable snapshot cache size effectively zero on non-frozen nodes #70846
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
Conversation
This commits makes the shared_cache searchable snapshot cache size setting resolve to 0 for nodes that to do have the `data_frozen` node role. It turns out there is not a simple way to do this with the existing Settings infrastructure. Given that setting this on non-frozen nodes is already deprecated (and already outputs a deprecated log message), After talking to Ryan, we agreed it was better to handle this upon reading the setting rather than adding anything to the `Settings` code for only this purpose. I also attempted to make the shared cache setting private (so it actually couldn't be retrieved outside of the static method), however, it's still needed in the `SearchableSnapshots` plugin interface, so without moving it there, it has to remain public (I can move it there if we decide that would be better). Tangentially related to elastic#70341
|
Pinging @elastic/es-distributed (Team:Distributed) |
This commit converts the index metadata of searchable snapshot indices using the `shared_cache` storage type to: - Remove all the `index.routing.allocation.(include|exclude|require)._tier` settings - Sets `index.routing.allocation.include._tier_preference` to `data_frozen` automatically when the index metadata is read This is in preperation to enforcing that the `_tier_preference` setting is always set to `data_frozen` for shared cache SBIs. Relates to elastic#70846, elastic#71013, elastic#70786, elastic#70141
|
I think that we can do this in the existing settings infrastructure by overriding |
|
Here's an untested patch that provides the concept: diff --git a/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/FrozenCacheService.java b/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/FrozenCacheService.java
index b1ebbaa5cdf..ac8ca7a3e66 100644
--- a/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/FrozenCacheService.java
+++ b/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/FrozenCacheService.java
@@ -117,7 +117,20 @@ public class FrozenCacheService implements Releasable {
},
Setting.Property.NodeScope
- );
+ ) {
+
+ @Override
+ public ByteSizeValue get(final Settings settings) {
+ final ByteSizeValue value = super.get(settings);
+ final List<DiscoveryNodeRole> roles = NodeRoleSettings.NODE_ROLES_SETTING.get(settings);
+ if (DataTier.isFrozenNode(Set.of(roles.toArray(DiscoveryNodeRole[]::new)))) {
+ return value;
+ } else {
+ return ByteSizeValue.ZERO;
+ }
+ }
+
+ };
public static final Setting<ByteSizeValue> FROZEN_CACHE_RECOVERY_RANGE_SIZE_SETTING = Setting.byteSizeSetting(
SHARED_CACHE_SETTINGS_PREFIX + "recovery_range_size",
diff --git a/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/xpack/searchablesnapshots/cache/FrozenCacheServiceTests.java b/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/xpack/searchablesnapshots/cache/FrozenCacheServiceTests.java
index 499b0cf4fce..d71d31cfb0f 100644
--- a/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/xpack/searchablesnapshots/cache/FrozenCacheServiceTests.java
+++ b/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/xpack/searchablesnapshots/cache/FrozenCacheServiceTests.java
@@ -10,6 +10,7 @@ package org.elasticsearch.xpack.searchablesnapshots.cache;
import org.elasticsearch.cluster.coordination.DeterministicTaskQueue;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.env.Environment;
import org.elasticsearch.env.TestEnvironment;
import org.elasticsearch.index.shard.ShardId;
@@ -26,6 +27,7 @@ import java.nio.file.Path;
import java.util.Set;
import static org.elasticsearch.node.Node.NODE_NAME_SETTING;
+import static org.hamcrest.Matchers.is;
public class FrozenCacheServiceTests extends ESTestCase {
@@ -213,6 +215,24 @@ public class FrozenCacheServiceTests extends ESTestCase {
);
}
+ public void testCacheSizeClampsToZeroOnNonFrozenNodes() {
+ DiscoveryNode.setAdditionalRoles(
+ Set.of(DataTier.DATA_HOT_NODE_ROLE, DataTier.DATA_WARM_NODE_ROLE, DataTier.DATA_COLD_NODE_ROLE, DataTier.DATA_FROZEN_NODE_ROLE)
+ );
+ final Settings settings = Settings.builder()
+ .put(FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(), "500b")
+ .put(FrozenCacheService.SNAPSHOT_CACHE_REGION_SIZE_SETTING.getKey(), "100b")
+ .putList(NodeRoleSettings.NODE_ROLES_SETTING.getKey(), DataTier.DATA_HOT_NODE_ROLE.roleName())
+ .build();
+ final ByteSizeValue value = FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.get(settings);
+ assertThat(value, is(ByteSizeValue.ZERO));
+ assertWarnings(
+ "setting ["
+ + FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey()
+ + "] to be positive [500b] on node without the data_frozen role is deprecated, roles are [data_hot]"
+ );
+ }
+
private static CacheKey generateCacheKey() {
return new CacheKey(
randomAlphaOfLength(10), |
jasontedor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a suggestion.
This commit converts the index metadata of searchable snapshot indices using the `shared_cache` storage type to: - Remove all the `index.routing.allocation.(include|exclude|require)._tier` settings - Sets `index.routing.allocation.include._tier_preference` to `data_frozen` automatically when the index metadata is read This is in preperation to enforcing that the `_tier_preference` setting is always set to `data_frozen` for shared cache SBIs. Relates to #70846, #71013, #70786, #70141
This commit converts the index metadata of searchable snapshot indices using the `shared_cache` storage type to: - Remove all the `index.routing.allocation.(include|exclude|require)._tier` settings - Sets `index.routing.allocation.include._tier_preference` to `data_frozen` automatically when the index metadata is read This is in preperation to enforcing that the `_tier_preference` setting is always set to `data_frozen` for shared cache SBIs. Relates to elastic#70846, elastic#71013, elastic#70786, elastic#70141
This commits makes the shared_cache searchable snapshot cache size setting resolve to 0 for nodes that to do have the data_frozen node role. Tangentially related to elastic#70341 Supercedes elastic#70846
|
@jasontedor yep that's a much better solution. I'm closing this in favor of #71134 which only targets 7.x (since you already have a PR for removing the leniency on master up) |
…#71129) This commit converts the index metadata of searchable snapshot indices using the `shared_cache` storage type to: - Remove all the `index.routing.allocation.(include|exclude|require)._tier` settings - Sets `index.routing.allocation.include._tier_preference` to `data_frozen` automatically when the index metadata is read This is in preperation to enforcing that the `_tier_preference` setting is always set to `data_frozen` for shared cache SBIs. Relates to #70846, #71013, #70786, #70141
…des (#71134) * Make searchable snapshot cache size effectively zero on non-frozen nodes This commits makes the shared_cache searchable snapshot cache size setting resolve to 0 for nodes that to do have the data_frozen node role. Tangentially related to #70341 Supercedes #70846 * Update message in HasFrozenCacheAllocationDecider
This commits makes the shared_cache searchable snapshot cache size setting resolve to 0 for nodes
that to do have the
data_frozennode role.It turns out there is not a simple way to do this with the existing Settings infrastructure. Given
that setting this on non-frozen nodes is already deprecated (and already outputs a deprecated log
message), After talking to Ryan, we agreed it was better to handle this upon reading the setting
rather than adding anything to the
Settingscode for only this purpose.I also attempted to make the shared cache setting private (so it actually couldn't be retrieved
outside of the static method), however, it's still needed in the
SearchableSnapshotsplugininterface, so without moving it there, it has to remain public (I can move it there if we decide
that would be better).
Tangentially related to #70341