From 9181668edf7186ad15d06d8fc528c7a1408b9883 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Thu, 14 Mar 2019 08:56:19 -0400 Subject: [PATCH] Stop returning cluster state size by default (#40016) Computing the compressed size of the cluster state on every invocation of cluster:monitor/state action is expensive, and the value of this field is dubious anyway. Therefore we want to remove computing this field. As a first step, we stop computing and return this field by default. To avoid breaking users, we will give them a system property to use to tide them over until the next major release when we will actually remove this field. This comes with a deprecation warning too, and the backport to the appropriate minor will also include a note in the migration guide. There will be a follow-up to remove this field in the next major version. --- qa/cluster-state-size/build.gradle | 33 ++++++++++ .../ClusterStateSizeRestIT.java | 62 +++++++++++++++++++ .../resources/rest-api-spec/test/10_basic.yml | 15 +++++ .../test/cluster.state/10_basic.yml | 11 ++-- .../cluster/state/ClusterStateResponse.java | 1 + .../state/TransportClusterStateAction.java | 33 +++++++++- .../admin/cluster/RestClusterStateAction.java | 9 ++- .../cluster/GetClusterStateTests.java | 6 +- .../rest/yaml/ESClientYamlSuiteTestCase.java | 10 ++- 9 files changed, 166 insertions(+), 14 deletions(-) create mode 100644 qa/cluster-state-size/build.gradle create mode 100644 qa/cluster-state-size/src/test/java/org/elasticsearch/cluster_state_size/ClusterStateSizeRestIT.java create mode 100644 qa/cluster-state-size/src/test/resources/rest-api-spec/test/10_basic.yml diff --git a/qa/cluster-state-size/build.gradle b/qa/cluster-state-size/build.gradle new file mode 100644 index 00000000000..b2b16855d44 --- /dev/null +++ b/qa/cluster-state-size/build.gradle @@ -0,0 +1,33 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import org.elasticsearch.gradle.test.RestIntegTestTask + +apply plugin: 'elasticsearch.standalone-test' + +task integTest(type: RestIntegTestTask) { + mustRunAfter(precommit) +} + +integTestCluster { + systemProperty 'es.cluster_state.size', 'true' +} + +unitTest.enabled = false +check.dependsOn integTest diff --git a/qa/cluster-state-size/src/test/java/org/elasticsearch/cluster_state_size/ClusterStateSizeRestIT.java b/qa/cluster-state-size/src/test/java/org/elasticsearch/cluster_state_size/ClusterStateSizeRestIT.java new file mode 100644 index 00000000000..045c3cdaddd --- /dev/null +++ b/qa/cluster-state-size/src/test/java/org/elasticsearch/cluster_state_size/ClusterStateSizeRestIT.java @@ -0,0 +1,62 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.cluster_state_size; + +import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; +import org.elasticsearch.Version; +import org.elasticsearch.client.RequestOptions; +import org.elasticsearch.test.rest.yaml.ClientYamlTestCandidate; +import org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase; + +import java.util.Collections; + +public class ClusterStateSizeRestIT extends ESClientYamlSuiteTestCase { + + public ClusterStateSizeRestIT(final ClientYamlTestCandidate candidate) { + super(candidate); + } + + @ParametersFactory + public static Iterable parameters() throws Exception { + return ESClientYamlSuiteTestCase.createParameters(); + } + + @Override + protected RequestOptions getCatNodesVersionMasterRequestOptions() { + final RequestOptions.Builder builder = RequestOptions.DEFAULT.toBuilder(); + final VersionSensitiveWarningsHandler handler = new VersionSensitiveWarningsHandler(Collections.singleton(Version.CURRENT)); + handler.current("es.cluster_state.size is deprecated and will be removed in 7.0.0"); + builder.setWarningsHandler(handler); + return builder.build(); + } + + @Override + protected boolean preserveClusterSettings() { + // we did not add any cluster settings, we are the only test using this cluster, and trying to clean these will trigger a warning + return true; + } + + @Override + protected boolean preserveTemplatesUponCompletion() { + // we did not add any templates, we are the only test using this cluster, and trying to clean these will trigger a warning + return true; + } + +} diff --git a/qa/cluster-state-size/src/test/resources/rest-api-spec/test/10_basic.yml b/qa/cluster-state-size/src/test/resources/rest-api-spec/test/10_basic.yml new file mode 100644 index 00000000000..529847b32bd --- /dev/null +++ b/qa/cluster-state-size/src/test/resources/rest-api-spec/test/10_basic.yml @@ -0,0 +1,15 @@ +--- +"get cluster state returns cluster state size with human readable format": + - skip: + reason: deprecation was added in 6.7.0 + features: "warnings" + + - do: + warnings: + - 'es.cluster_state.size is deprecated and will be removed in 7.0.0' + cluster.state: + human: true + + - is_true: master_node + - gte: { compressed_size_in_bytes: 50 } + - is_true: compressed_size diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.state/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.state/10_basic.yml index ceed71c18e4..f7a6b0455b1 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.state/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.state/10_basic.yml @@ -6,14 +6,17 @@ - is_true: master_node --- -"get cluster state returns cluster state size with human readable format": +"get cluster state does not return cluster state size by default": + - skip: + version: " - 7.0.99" + reason: "backwards compatibility break in 6.7.0 removes compressed_size* from the response" - do: cluster.state: human: true - is_true: master_node - - gte: { compressed_size_in_bytes: 50 } - - is_true: compressed_size + - is_false: compressed_size_in_bytes + - is_false: compressed_size --- "get cluster state returns cluster_uuid at the top level": @@ -27,5 +30,3 @@ - is_true: cluster_uuid - is_true: master_node - - gte: { compressed_size_in_bytes: 50 } - - is_true: compressed_size diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/state/ClusterStateResponse.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/state/ClusterStateResponse.java index fc4eb04fada..24f9c49f40b 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/state/ClusterStateResponse.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/state/ClusterStateResponse.java @@ -75,6 +75,7 @@ public class ClusterStateResponse extends ActionResponse { * of the cluster state as it would be transmitted over the network during * intra-node communication. */ + @Deprecated public ByteSizeValue getTotalCompressedSize() { return totalCompressedSize; } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/state/TransportClusterStateAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/state/TransportClusterStateAction.java index 3eb3f1746b4..55acc6d095c 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/state/TransportClusterStateAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/state/TransportClusterStateAction.java @@ -20,6 +20,8 @@ package org.elasticsearch.action.admin.cluster.state; import com.carrotsearch.hppc.cursors.ObjectObjectCursor; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.elasticsearch.Version; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.ActionFilters; @@ -35,6 +37,7 @@ import org.elasticsearch.cluster.metadata.MetaData.Custom; import org.elasticsearch.cluster.routing.RoutingTable; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.inject.Inject; +import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.node.NodeClosedException; import org.elasticsearch.threadpool.ThreadPool; @@ -45,6 +48,24 @@ import java.util.function.Predicate; public class TransportClusterStateAction extends TransportMasterNodeReadAction { + private final Logger logger = LogManager.getLogger(getClass()); + private final DeprecationLogger deprecationLogger = new DeprecationLogger(logger); + + public static final boolean CLUSTER_STATE_SIZE; + + static { + final String property = System.getProperty("es.cluster_state.size"); + if (property == null) { + CLUSTER_STATE_SIZE = false; + } else { + final boolean clusterStateSize = Boolean.parseBoolean(property); + if (clusterStateSize) { + CLUSTER_STATE_SIZE = true; + } else { + throw new IllegalArgumentException("es.cluster_state.size can only be unset or [true] but was [" + property + "]"); + } + } + } @Inject public TransportClusterStateAction(TransportService transportService, ClusterService clusterService, @@ -183,8 +204,16 @@ public class TransportClusterStateAction extends TransportMasterNodeReadAction readVersionsFromCatNodes(RestClient restClient) throws IOException { + private Tuple readVersionsFromCatNodes(RestClient restClient) throws IOException { // we simply go to the _cat/nodes API and parse all versions in the cluster - Request request = new Request("GET", "/_cat/nodes"); + final Request request = new Request("GET", "/_cat/nodes"); request.addParameter("h", "version,master"); + request.setOptions(getCatNodesVersionMasterRequestOptions()); Response response = restClient.performRequest(request); ClientYamlTestResponse restTestResponse = new ClientYamlTestResponse(response); String nodesCatResponse = restTestResponse.getBodyAsString(); @@ -319,6 +321,10 @@ public abstract class ESClientYamlSuiteTestCase extends ESRestTestCase { return new Tuple<>(version, masterVersion); } + protected RequestOptions getCatNodesVersionMasterRequestOptions() { + return RequestOptions.DEFAULT; + } + public void test() throws IOException { //skip test if it matches one of the blacklist globs for (BlacklistedPathPatternMatcher blacklistedPathMatcher : blacklistPathMatchers) {