Faster GET _cluster/settings API #86405

grcevski · 2022-05-03T19:38:16Z

The existing API to retrieve the cluster settings relies on
pulling in the full cluster state, which can be very expensive.

This change adds a dedicated cluster settings API, avoiding
serializing the full cluster state.

Closes #82342

The existing API to retrieve the cluster settings relies on pulling in the full cluster state, which can be very expensive. This change adds a dedicated cluster settings API, avoiding serializing the full cluster state. Closes elastic#82342

elasticmachine · 2022-05-03T19:38:18Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

elasticsearchmachine · 2022-05-03T19:38:40Z

Hi @grcevski, I've created a changelog YAML for you.

grcevski · 2022-05-03T19:53:39Z

.../src/main/java/org/elasticsearch/action/admin/cluster/settings/ClusterGetSettingsAction.java

+        }
+
+        public Settings settings() {
+            return Settings.builder().put(persistentSettings).put(transientSettings).build();


I'm not sure this is the right way to get the combined settings, it didn't make sense to double serialize across the transport protocol.

It looks right:

elasticsearch/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java

Line 1875 in 5791567

Settings.builder().put(persistentSettings).put(transientSettings).build(),

However I think it would be fine (preferable even) to send the combined settings separately from the transient and persistent ones, rather than duplicating this logic and risking getting it wrong.

…evski/elasticsearch into enhancement/cheaper_settings_request

DaveCTurner

Looks good. I left some small comments.

In terms of tests, yes this seems mostly pretty well covered. I think we should have some tests that verify that we haven't mixed up the persistent and transient settings anywhere along the way, and that the new transport action applies settings filters correctly. Also I think we should have an AbstractWireSerializingTestCase<ClusterGetSettingsAction.Response> (including a mutateInstance implementation) to ensure it round-trips properly.

DaveCTurner · 2022-05-04T13:30:54Z

.../src/main/java/org/elasticsearch/action/admin/cluster/settings/ClusterGetSettingsAction.java

+        }
+
+        public Settings settings() {
+            return Settings.builder().put(persistentSettings).put(transientSettings).build();


It looks right:

elasticsearch/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java

Line 1875 in 5791567

Settings.builder().put(persistentSettings).put(transientSettings).build(),

However I think it would be fine (preferable even) to send the combined settings separately from the transient and persistent ones, rather than duplicating this logic and risking getting it wrong.

DaveCTurner · 2022-05-04T13:32:47Z

.../java/org/elasticsearch/action/admin/cluster/settings/TransportClusterGetSettingsAction.java

+        ActionListener<ClusterGetSettingsAction.Response> listener
+    ) throws Exception {
+        Metadata metadata = state.metadata();
+        listener.onResponse(new ClusterGetSettingsAction.Response(metadata.persistentSettings(), metadata.transientSettings()));


I think we should apply the node's SettingsFilter here. That's sort of a change in behaviour since we don't filter the settings in the cluster state today, but then again this is a security thing and in most of the other transport actions that send settings around we do filter them.

DaveCTurner · 2022-05-04T13:35:54Z

.../src/main/java/org/elasticsearch/action/admin/cluster/settings/ClusterGetSettingsAction.java

+public class ClusterGetSettingsAction extends ActionType<ClusterGetSettingsAction.Response> {
+
+    public static final ClusterGetSettingsAction INSTANCE = new ClusterGetSettingsAction();
+    public static final String NAME = "cluster:admin/settings/get";


I think this needs to be under cluster:monitor/* (maybe cluster:monitor/settings?) so that it's still permitted by clients that only have the monitor privilege.

Good idea, this way I can undo some of the changes I needed to do in xpack.

DaveCTurner · 2022-05-04T13:36:38Z

.../src/main/java/org/elasticsearch/action/admin/cluster/settings/ClusterGetSettingsAction.java

+    /**
+     * Response for cluster settings
+     */
+    public static class Response extends ActionResponse implements ToXContentObject {


Does this need to be implements ToXContentObject? I think we don't expose it as XContent directly, it's always converted to a RestClusterGetSettingsResponse first.

.../src/main/java/org/elasticsearch/action/admin/cluster/settings/ClusterGetSettingsAction.java

DaveCTurner · 2022-05-04T13:42:57Z

.../src/main/java/org/elasticsearch/action/admin/cluster/settings/ClusterGetSettingsAction.java

+    /**
+     * Cluster get settings request builder
+     */
+    public static class RequestBuilder extends MasterNodeReadOperationRequestBuilder<Request, Response, RequestBuilder> {


I wonder, do we really need all this client-facing machinery? AFAIK this pattern was good for the transport client, but we have no transport client any more so I think we can just call the action directly in the one place it's currently used.

.../java/org/elasticsearch/action/admin/cluster/settings/TransportClusterGetSettingsAction.java

DaveCTurner · 2022-05-04T13:46:41Z

server/src/main/java/org/elasticsearch/client/internal/ClusterAdminClient.java

+     * @param request The cluster settings request
+     * @return The result future
+     */
+    ActionFuture<ClusterGetSettingsAction.Response> clusterSettings(ClusterGetSettingsAction.Request request);


Similarly here, I think this stuff is all unused in practice and best dropped.

.../src/main/java/org/elasticsearch/rest/action/admin/cluster/RestClusterGetSettingsAction.java

DaveCTurner · 2022-05-04T13:48:44Z

.../src/main/java/org/elasticsearch/rest/action/admin/cluster/RestClusterGetSettingsAction.java

+        clusterStateRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterStateRequest.masterNodeTimeout()));
+        return channel -> client.admin()
+            .cluster()
+            .state(clusterStateRequest, new RestToXContentListener<RestClusterGetSettingsResponse>(channel).map(response -> {


I think it would be slightly nicer to use the same ActionListener<ClusterGetSettingsAction.Response> for both the legacy and the regular case, with a .map(clusterState -> new ClusterGetSettingsAction.Response(...)) to do the extra conversion step in the legacy case.

…ttings/TransportClusterGetSettingsAction.java disable circuit breakers Co-authored-by: David Turner <[email protected]>

…er/RestClusterGetSettingsAction.java Change onOrAfter to before. Co-authored-by: David Turner <[email protected]>

…evski/elasticsearch into enhancement/cheaper_settings_request

We use the minimum node version in the cluster state to make decisions about backwards compatibility (e.g. to choose newer actions in the REST layer only if all nodes will support it). Once the cluster is fully formed we reject attempts by older nodes to join the cluster so that the minimum node version only ever increases, which makes backwards-compatibility decisions safe. However, it's possible that the REST layer will make decisions about backwards compatibility before the cluster is fully formed. In this state, older nodes may still join the cluster and may therefore see actions that they do not understand. With this commit we report no nodes to the REST layer until the cluster is fully-formed, and change the minimum node version in an empty cluster to be the minimum compatible version. This means the REST layer will operate in a maximally-compatible mode until the cluster is formed. Relates elastic#86405

We use the minimum node version in the cluster state to make decisions about backwards compatibility (e.g. to choose newer actions in the REST layer only if all nodes will support it). Once the cluster is fully formed we reject attempts by older nodes to join the cluster so that the minimum node version only ever increases, which makes backwards-compatibility decisions safe. However, it's possible that the REST layer will make decisions about backwards compatibility before the cluster is fully formed. In this state, older nodes may still join the cluster and may therefore see actions that they do not understand. With this commit we report no nodes to the REST layer until the cluster is fully-formed, and change the minimum node version in an empty cluster to be the minimum compatible version. This means the REST layer will operate in a maximally-compatible mode until the cluster is formed. Relates #86405

...r/src/test/java/org/elasticsearch/action/admin/cluster/settings/ClusterGetSettingsTests.java

…ttings/ClusterGetSettingsTests.java Co-authored-by: David Turner <[email protected]>

DaveCTurner

LGTM

DaveCTurner · 2022-05-09T09:03:11Z

server/src/main/java/org/elasticsearch/client/internal/ClusterAdminClient.java

@@ -756,4 +756,5 @@ public interface ClusterAdminClient extends ElasticsearchClient {
     * Delete specified dangling indices.
     */
    ActionFuture<AcknowledgedResponse> deleteDanglingIndex(DeleteDanglingIndexRequest request);
+


Nit: this file only contains whitespace changes now

DaveCTurner · 2022-05-09T09:05:32Z

.../src/main/java/org/elasticsearch/rest/action/admin/cluster/RestClusterGetSettingsAction.java

+            ClusterGetSettingsAction.INSTANCE,
+            clusterSettingsRequest,
+            new RestToXContentListener<RestClusterGetSettingsResponse>(channel).map(
+                r -> response(r, renderDefaults, settingsFilter, clusterSettings, settings)


I'd still sort of like to reduce the duplication here between the two branches but I see it's not so simple because we have to consume the request params before we get hold of the channel. Not a blocking suggestion.

joegallo · 2022-05-11T18:01:32Z

Related to #77466

Nikola Grcevski added 2 commits May 3, 2022 14:57

Implement faster GET _cluster/settings API

75d179c

The existing API to retrieve the cluster settings relies on pulling in the full cluster state, which can be very expensive. This change adds a dedicated cluster settings API, avoiding serializing the full cluster state. Closes elastic#82342

Reuse filtering logic

8a88827

grcevski added >enhancement :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team v8.3.0 labels May 3, 2022

Update docs/changelog/86405.yaml

25f0240

grcevski commented May 3, 2022

View reviewed changes

Nikola Grcevski added 3 commits May 3, 2022 19:07

Fix privileges

022864b

Merge branch 'enhancement/cheaper_settings_request' of github.com:grc…

5666ecf

…evski/elasticsearch into enhancement/cheaper_settings_request

More security fixes

68394fd

DaveCTurner requested changes May 4, 2022

View reviewed changes

grcevski and others added 10 commits May 4, 2022 10:33

Update server/src/main/java/org/elasticsearch/action/admin/cluster/se…

7d91858

…ttings/TransportClusterGetSettingsAction.java disable circuit breakers Co-authored-by: David Turner <[email protected]>

Update server/src/main/java/org/elasticsearch/rest/action/admin/clust…

5fce1b3

…er/RestClusterGetSettingsAction.java Change onOrAfter to before. Co-authored-by: David Turner <[email protected]>

Apply review suggestions

49f2703

Merge branch 'enhancement/cheaper_settings_request' of github.com:grc…

2abab4e

…evski/elasticsearch into enhancement/cheaper_settings_request

Review changes

0499aab

Move assert earlier

8ff26f3

Merge branch 'master' into enhancement/cheaper_settings_request

bdca67c

Add settings filters

8a13ecd

Basic tests

fad4113

Merge branch 'master' into enhancement/cheaper_settings_request

6c5f6fb

DaveCTurner mentioned this pull request May 5, 2022

Fix min node version before state recovery #86482

Merged

Add wire serialization test

8aac2df

DaveCTurner reviewed May 7, 2022

View reviewed changes

...r/src/test/java/org/elasticsearch/action/admin/cluster/settings/ClusterGetSettingsTests.java Outdated Show resolved Hide resolved

Update server/src/test/java/org/elasticsearch/action/admin/cluster/se…

7019207

…ttings/ClusterGetSettingsTests.java Co-authored-by: David Turner <[email protected]>

Nikola Grcevski added 2 commits May 9, 2022 11:00

Resolve compile error

c870408

Merge branch 'master' into enhancement/cheaper_settings_request

be83573

DaveCTurner approved these changes May 9, 2022

View reviewed changes

Review suggestions

4d32a3f

grcevski merged commit 4b536c6 into elastic:master May 9, 2022

grcevski deleted the enhancement/cheaper_settings_request branch May 9, 2022 10:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faster GET _cluster/settings API #86405

Faster GET _cluster/settings API #86405

grcevski commented May 3, 2022

elasticmachine commented May 3, 2022

elasticsearchmachine commented May 3, 2022

grcevski May 3, 2022

DaveCTurner May 4, 2022

DaveCTurner left a comment

DaveCTurner May 4, 2022

DaveCTurner May 4, 2022

DaveCTurner May 4, 2022

grcevski May 4, 2022

DaveCTurner May 4, 2022

DaveCTurner May 4, 2022

DaveCTurner May 4, 2022

DaveCTurner May 4, 2022

DaveCTurner left a comment

DaveCTurner May 9, 2022

DaveCTurner May 9, 2022

joegallo commented May 11, 2022

Faster GET _cluster/settings API #86405

Faster GET _cluster/settings API #86405

Conversation

grcevski commented May 3, 2022

elasticmachine commented May 3, 2022

elasticsearchmachine commented May 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joegallo commented May 11, 2022