Deprecate setting shared cache on non-frozen nodes by jasontedor · Pull Request #70341 · elastic/elasticsearch

jasontedor · 2021-03-11T22:30:47Z

This commit deprecates the ability to set the shared cache to be positive on non-frozen nodes.

elasticmachine · 2021-03-11T22:30:50Z

Pinging @elastic/es-distributed (Team:Distributed)

dakrone

Generally LGTM, but I did leave some comments and questions

dakrone · 2021-03-11T22:37:25Z

...hots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/FrozenCacheService.java

+                if (value.getBytes() > 0) {
+                    @SuppressWarnings("unchecked") final List<DiscoveryNodeRole> roles =
+                        (List<DiscoveryNodeRole>) settings.get(NodeRoleSettings.NODE_ROLES_SETTING);
+                    final DiscoveryNode fake = new DiscoveryNode(


Rather than constructing this fake DiscoveryNode, what about making DataTier.isFrozenNode(...) invoke a separate static method that takes a Set<DiscoveryNodeRole> so that method could be used directly? It feels weird to construct a whole fake node just for this purpose.

It felt weird to add a whole method just for this purpose. 🤷‍♀️

Especially since it's hard to come up with a good name for the method. isFrozenNode isn't great because it's not testing a node, hasFrozenRole isn't great because it can also include the data role and should return true.

I pushed 892f575.

dakrone · 2021-03-11T22:40:24Z

...hots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/FrozenCacheService.java

+            @Override
+            public void validate(final ByteSizeValue value, final Map<Setting<?>, Object> settings) {
+                if (value.getBytes() > 0) {
+                    @SuppressWarnings("unchecked") final List<DiscoveryNodeRole> roles =


I'd feel better if we had an assert that roles was not null so we fail fast, or if we want to be lenient (which we probably don't) do settings.get(NodeRoleSettings.NODE_ROLES_SETTING, Collections.emptyList())

I don't think roles can be null here. We don't allow explicitly null-valued settings, since that represents unset, and therefore the default value.

dakrone · 2021-03-11T22:42:06Z

...hots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/FrozenCacheService.java

+
+            @Override
+            public void validate(final ByteSizeValue value) {
+


Should we disallow a ByteSizeValue of -1b since that is a valid ByteSizeValue object here?

It feels like the setting should be either 0b or > 0b, but not -1.

I pushed 3f02383.

dakrone · 2021-03-11T22:43:05Z

x-pack/plugin/searchable-snapshots/src/main/plugin-metadata/plugin-security.codebases

@@ -0,0 +1 @@
+preallocate: org.elasticsearch.xpack.searchablesnapshots.preallocate.Preallocate


This seems unrelated to this change?

It's needed to be able to run the relevant unit tests inside of IntelliJ.

...hots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/FrozenCacheService.java

…arch/xpack/searchablesnapshots/cache/FrozenCacheService.java Co-authored-by: Lee Hinman <dakrone@users.noreply.github.com>

dakrone

Still LGTM

...src/test/java/org/elasticsearch/xpack/searchablesnapshots/cache/FrozenCacheServiceTests.java

…arch/xpack/searchablesnapshots/cache/FrozenCacheServiceTests.java Co-authored-by: Lee Hinman <dakrone@users.noreply.github.com>

This commit deprecates the ability to set the shared cache to be positive on non-frozen nodes.

…n only We recently deprecated the ability to set the shared cache outside of the frozen tier, and eventually will only allow shared_cache searchable snapshots to existing on nodes with the `data_frozen` role. This commit changes those searchable snapshots to only be allocated on the frozen tier, rather than an ordered list of frozen/cold/warm/hot. Relates to elastic#70341

…n only (#70786) We recently deprecated the ability to set the shared cache outside of the frozen tier, and eventually will only allow shared_cache searchable snapshots to existing on nodes with the `data_frozen` role. This commit changes those searchable snapshots to only be allocated on the frozen tier, rather than an ordered list of frozen/cold/warm/hot. Relates to #70341

…n only (elastic#70786) We recently deprecated the ability to set the shared cache outside of the frozen tier, and eventually will only allow shared_cache searchable snapshots to existing on nodes with the `data_frozen` role. This commit changes those searchable snapshots to only be allocated on the frozen tier, rather than an ordered list of frozen/cold/warm/hot. Relates to elastic#70341

… frozen only (#70786) (#70818) We recently deprecated the ability to set the shared cache outside of the frozen tier, and eventually will only allow shared_cache searchable snapshots to existing on nodes with the `data_frozen` role. This commit changes those searchable snapshots to only be allocated on the frozen tier, rather than an ordered list of frozen/cold/warm/hot. Relates to #70341

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

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

…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

…he.size` (#77727) We deprecated the `xpack.searchable.snapshot.shared_cache.size` setting on non-frozen nodes in 7.12 with PR #70341. However, we didn't add a related item to the 7.12 deprecation docs. This adds the missing item. Relates to #71013.

…he.size` (#77727) (#77762) We deprecated the `xpack.searchable.snapshot.shared_cache.size` setting on non-frozen nodes in 7.12 with PR #70341. However, we didn't add a related item to the 7.12 deprecation docs. This adds the missing item. Relates to #71013.

…he.size` (#77727) (#77763) We deprecated the `xpack.searchable.snapshot.shared_cache.size` setting on non-frozen nodes in 7.12 with PR #70341. However, we didn't add a related item to the 7.12 deprecation docs. This adds the missing item. Relates to #71013.

…he.size` (#77727) (#77764) We deprecated the `xpack.searchable.snapshot.shared_cache.size` setting on non-frozen nodes in 7.12 with PR #70341. However, we didn't add a related item to the 7.12 deprecation docs. This adds the missing item. Relates to #71013.

…he.size` (#77727) (#77765) We deprecated the `xpack.searchable.snapshot.shared_cache.size` setting on non-frozen nodes in 7.12 with PR #70341. However, we didn't add a related item to the 7.12 deprecation docs. This adds the missing item. Relates to #71013. # Conflicts: # docs/reference/migration/migrate_7_12.asciidoc

Deprecate setting shared cache on non-frozen nodes

edd7742

This commit deprecates the ability to set the shared cache to be positive on non-frozen nodes.

jasontedor added >non-issue :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.0.0 v7.12.0 v7.13.0 labels Mar 11, 2021

jasontedor requested review from dakrone, henningandersen and ywelsch March 11, 2021 22:30

elasticmachine added the Team:Distributed Meta label for distributed team. label Mar 11, 2021

Checkstyle!

6ca31f0

dakrone approved these changes Mar 11, 2021

View reviewed changes

jasontedor and others added 3 commits March 11, 2021 17:46

Update x-pack/plugin/searchable-snapshots/src/main/java/org/elasticse…

97daaaf

…arch/xpack/searchablesnapshots/cache/FrozenCacheService.java Co-authored-by: Lee Hinman <dakrone@users.noreply.github.com>

Add simple method

892f575

Validation

3f02383

jasontedor requested a review from dakrone March 11, 2021 23:15

dakrone approved these changes Mar 11, 2021

View reviewed changes

...src/test/java/org/elasticsearch/xpack/searchablesnapshots/cache/FrozenCacheServiceTests.java Outdated Show resolved Hide resolved

jasontedor and others added 2 commits March 11, 2021 18:19

Update x-pack/plugin/searchable-snapshots/src/test/java/org/elasticse…

12a624c

…arch/xpack/searchablesnapshots/cache/FrozenCacheServiceTests.java Co-authored-by: Lee Hinman <dakrone@users.noreply.github.com>

Fix formatting

07928de

jasontedor merged commit 3a814a2 into elastic:master Mar 12, 2021

jasontedor added a commit that referenced this pull request Mar 12, 2021

Deprecate setting shared cache on non-frozen nodes (#70341)

13c375b

This commit deprecates the ability to set the shared cache to be positive on non-frozen nodes.

jasontedor added a commit that referenced this pull request Mar 12, 2021

Deprecate setting shared cache on non-frozen nodes (#70341)

54ca49f

This commit deprecates the ability to set the shared cache to be positive on non-frozen nodes.

jasontedor deleted the deprecate-shared-cache-on-non-frozen-nodes branch March 12, 2021 01:27

dakrone mentioned this pull request Mar 23, 2021

Change tier preference for shared_cache searchable snapshots to frozen only #70786

Merged

dakrone mentioned this pull request Mar 24, 2021

Make searchable snapshot cache size effectively zero on non-frozen nodes #70846

Closed

jasontedor mentioned this pull request Mar 29, 2021

Remove frozen cache setting leniency #71013

Merged

dakrone mentioned this pull request Mar 31, 2021

Make searchable snapshot cache size effectively zero on non-frozen nodes #71134

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

jrodewig mentioned this pull request Sep 14, 2021

[DOCS] Add deprecation docs for xpack.searchable.snapshot.shared_cache.size #77727

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate setting shared cache on non-frozen nodes#70341

Deprecate setting shared cache on non-frozen nodes#70341
jasontedor merged 7 commits intoelastic:masterfrom
jasontedor:deprecate-shared-cache-on-non-frozen-nodes

jasontedor commented Mar 11, 2021

Uh oh!

elasticmachine commented Mar 11, 2021

Uh oh!

dakrone left a comment

Uh oh!

dakrone Mar 11, 2021

Uh oh!

jasontedor Mar 11, 2021

Uh oh!

dakrone Mar 11, 2021

Uh oh!

jasontedor Mar 11, 2021

Uh oh!

dakrone Mar 11, 2021

Uh oh!

jasontedor Mar 11, 2021

Uh oh!

dakrone Mar 11, 2021

Uh oh!

jasontedor Mar 11, 2021

Uh oh!

Uh oh!

dakrone left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		@@ -0,0 +1 @@
		preallocate: org.elasticsearch.xpack.searchablesnapshots.preallocate.Preallocate

Conversation

jasontedor commented Mar 11, 2021

Uh oh!

elasticmachine commented Mar 11, 2021

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants