Skip to content

Commit

Permalink
Clean up request/response for get-cluster-settings
Browse files Browse the repository at this point in the history
Today there are request and response objects for the
get-cluster-settings action but the request is unused and the response
is only used in the REST layer. This commit removes the unused request
and renames the response to reflect that it's not a transport-layer
response. It also tidies a few things up in this area, removing the
unused `ActionResponse` superclass, making its fields final, and
replacing the overly-general `RestBuilderListener` with a regular
`RestToXContentListener` in the REST action.

Relates elastic#82342 because to resolve that issue we will want to introduce
transport-layer request/response classes, and the classes involved in
this commit are in the way of that change.
  • Loading branch information
DaveCTurner committed May 1, 2022
1 parent a5f1782 commit 5326e7b
Show file tree
Hide file tree
Showing 6 changed files with 39 additions and 97 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

import org.apache.http.util.EntityUtils;
import org.elasticsearch.Version;
import org.elasticsearch.action.admin.cluster.settings.ClusterGetSettingsResponse;
import org.elasticsearch.action.admin.cluster.settings.RestClusterGetSettingsResponse;
import org.elasticsearch.client.Request;
import org.elasticsearch.client.Response;
import org.elasticsearch.client.ResponseException;
Expand Down Expand Up @@ -1849,8 +1849,7 @@ public void testTransportCompressionSetting() throws IOException {
final Request getSettingsRequest = new Request("GET", "/_cluster/settings");
final Response getSettingsResponse = client().performRequest(getSettingsRequest);
try (XContentParser parser = createParser(JsonXContent.jsonXContent, getSettingsResponse.getEntity().getContent())) {
final ClusterGetSettingsResponse clusterGetSettingsResponse = ClusterGetSettingsResponse.fromXContent(parser);
final Settings settings = clusterGetSettingsResponse.getPersistentSettings();
final Settings settings = RestClusterGetSettingsResponse.fromXContent(parser).getPersistentSettings();
assertThat(REMOTE_CLUSTER_COMPRESS.getConcreteSettingForNamespace("foo").get(settings), equalTo(Compression.Enabled.TRUE));
}
}
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@

package org.elasticsearch.action.admin.cluster.settings;

import org.elasticsearch.action.ActionResponse;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.xcontent.ConstructingObjectParser;
import org.elasticsearch.xcontent.ParseField;
Expand All @@ -28,22 +26,22 @@
* This response is specific to the REST client. {@link org.elasticsearch.action.admin.cluster.state.ClusterStateResponse}
* is used on the transport layer.
*/
public class ClusterGetSettingsResponse extends ActionResponse implements ToXContentObject {
public class RestClusterGetSettingsResponse implements ToXContentObject {

private Settings persistentSettings = Settings.EMPTY;
private Settings transientSettings = Settings.EMPTY;
private Settings defaultSettings = Settings.EMPTY;
private final Settings persistentSettings;
private final Settings transientSettings;
private final Settings defaultSettings;

static final String PERSISTENT_FIELD = "persistent";
static final String TRANSIENT_FIELD = "transient";
static final String DEFAULTS_FIELD = "defaults";

private static final ConstructingObjectParser<ClusterGetSettingsResponse, Void> PARSER = new ConstructingObjectParser<>(
private static final ConstructingObjectParser<RestClusterGetSettingsResponse, Void> PARSER = new ConstructingObjectParser<>(
"cluster_get_settings_response",
true,
a -> {
Settings defaultSettings = a[2] == null ? Settings.EMPTY : (Settings) a[2];
return new ClusterGetSettingsResponse((Settings) a[0], (Settings) a[1], defaultSettings);
return new RestClusterGetSettingsResponse((Settings) a[0], (Settings) a[1], defaultSettings);
}
);
static {
Expand All @@ -52,16 +50,10 @@ public class ClusterGetSettingsResponse extends ActionResponse implements ToXCon
PARSER.declareObject(optionalConstructorArg(), (p, c) -> Settings.fromXContent(p), new ParseField(DEFAULTS_FIELD));
}

public ClusterGetSettingsResponse(Settings persistentSettings, Settings transientSettings, Settings defaultSettings) {
if (persistentSettings != null) {
this.persistentSettings = persistentSettings;
}
if (transientSettings != null) {
this.transientSettings = transientSettings;
}
if (defaultSettings != null) {
this.defaultSettings = defaultSettings;
}
public RestClusterGetSettingsResponse(Settings persistentSettings, Settings transientSettings, Settings defaultSettings) {
this.persistentSettings = Objects.requireNonNullElse(persistentSettings, Settings.EMPTY);
this.transientSettings = Objects.requireNonNullElse(transientSettings, Settings.EMPTY);
this.defaultSettings = Objects.requireNonNullElse(defaultSettings, Settings.EMPTY);
}

/**
Expand Down Expand Up @@ -127,15 +119,15 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
return builder;
}

public static ClusterGetSettingsResponse fromXContent(XContentParser parser) {
public static RestClusterGetSettingsResponse fromXContent(XContentParser parser) {
return PARSER.apply(parser, null);
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
ClusterGetSettingsResponse that = (ClusterGetSettingsResponse) o;
RestClusterGetSettingsResponse that = (RestClusterGetSettingsResponse) o;
return Objects.equals(transientSettings, that.transientSettings)
&& Objects.equals(persistentSettings, that.persistentSettings)
&& Objects.equals(defaultSettings, that.defaultSettings);
Expand All @@ -150,7 +142,4 @@ public int hashCode() {
public String toString() {
return Strings.toString(this);
}

@Override
public void writeTo(StreamOutput out) throws IOException {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,17 @@

package org.elasticsearch.rest.action.admin.cluster;

import org.elasticsearch.action.admin.cluster.settings.ClusterGetSettingsResponse;
import org.elasticsearch.action.admin.cluster.settings.RestClusterGetSettingsResponse;
import org.elasticsearch.action.admin.cluster.state.ClusterStateRequest;
import org.elasticsearch.action.admin.cluster.state.ClusterStateResponse;
import org.elasticsearch.client.internal.Requests;
import org.elasticsearch.client.internal.node.NodeClient;
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.settings.SettingsFilter;
import org.elasticsearch.rest.BaseRestHandler;
import org.elasticsearch.rest.BytesRestResponse;
import org.elasticsearch.rest.RestRequest;
import org.elasticsearch.rest.RestResponse;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.rest.action.RestBuilderListener;
import org.elasticsearch.xcontent.ToXContent;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.rest.action.RestToXContentListener;

import java.io.IOException;
import java.util.List;
Expand Down Expand Up @@ -60,12 +54,14 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
final boolean renderDefaults = request.paramAsBoolean("include_defaults", false);
clusterStateRequest.local(request.paramAsBoolean("local", clusterStateRequest.local()));
clusterStateRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterStateRequest.masterNodeTimeout()));
return channel -> client.admin().cluster().state(clusterStateRequest, new RestBuilderListener<ClusterStateResponse>(channel) {
@Override
public RestResponse buildResponse(ClusterStateResponse response, XContentBuilder builder) throws Exception {
return new BytesRestResponse(RestStatus.OK, renderResponse(response.getState(), renderDefaults, builder, request));
}
});
return channel -> client.admin()
.cluster()
.state(
clusterStateRequest,
new RestToXContentListener<RestClusterGetSettingsResponse>(channel).map(
response -> response(response.getState(), renderDefaults, settingsFilter, clusterSettings, settings)
)
);
}

@Override
Expand All @@ -78,19 +74,14 @@ public boolean canTripCircuitBreaker() {
return false;
}

private XContentBuilder renderResponse(ClusterState state, boolean renderDefaults, XContentBuilder builder, ToXContent.Params params)
throws IOException {
return response(state, renderDefaults, settingsFilter, clusterSettings, settings).toXContent(builder, params);
}

static ClusterGetSettingsResponse response(
static RestClusterGetSettingsResponse response(
final ClusterState state,
final boolean renderDefaults,
final SettingsFilter settingsFilter,
final ClusterSettings clusterSettings,
final Settings settings
) {
return new ClusterGetSettingsResponse(
return new RestClusterGetSettingsResponse(
settingsFilter.filter(state.metadata().persistentSettings()),
settingsFilter.filter(state.metadata().transientSettings()),
renderDefaults ? settingsFilter.filter(clusterSettings.diff(state.metadata().settings(), settings)) : Settings.EMPTY
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
import java.io.IOException;
import java.util.function.Predicate;

public class ClusterGetSettingsResponseTests extends AbstractXContentTestCase<ClusterGetSettingsResponse> {
public class RestClusterGetSettingsResponseTests extends AbstractXContentTestCase<RestClusterGetSettingsResponse> {

@Override
protected ClusterGetSettingsResponse doParseInstance(XContentParser parser) throws IOException {
return ClusterGetSettingsResponse.fromXContent(parser);
protected RestClusterGetSettingsResponse doParseInstance(XContentParser parser) throws IOException {
return RestClusterGetSettingsResponse.fromXContent(parser);
}

@Override
Expand All @@ -28,17 +28,17 @@ protected boolean supportsUnknownFields() {
}

@Override
protected ClusterGetSettingsResponse createTestInstance() {
protected RestClusterGetSettingsResponse createTestInstance() {
Settings persistentSettings = ClusterUpdateSettingsResponseTests.randomClusterSettings(0, 2);
Settings transientSettings = ClusterUpdateSettingsResponseTests.randomClusterSettings(0, 2);
Settings defaultSettings = randomBoolean() ? ClusterUpdateSettingsResponseTests.randomClusterSettings(0, 2) : Settings.EMPTY;
return new ClusterGetSettingsResponse(persistentSettings, transientSettings, defaultSettings);
return new RestClusterGetSettingsResponse(persistentSettings, transientSettings, defaultSettings);
}

@Override
protected Predicate<String> getRandomFieldsExcludeFilter() {
return p -> p.startsWith(ClusterGetSettingsResponse.TRANSIENT_FIELD)
|| p.startsWith(ClusterGetSettingsResponse.PERSISTENT_FIELD)
|| p.startsWith(ClusterGetSettingsResponse.DEFAULTS_FIELD);
return p -> p.startsWith(RestClusterGetSettingsResponse.TRANSIENT_FIELD)
|| p.startsWith(RestClusterGetSettingsResponse.PERSISTENT_FIELD)
|| p.startsWith(RestClusterGetSettingsResponse.DEFAULTS_FIELD);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

package org.elasticsearch.rest.action.admin.cluster;

import org.elasticsearch.action.admin.cluster.settings.ClusterGetSettingsResponse;
import org.elasticsearch.action.admin.cluster.settings.RestClusterGetSettingsResponse;
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.metadata.Metadata;
import org.elasticsearch.common.settings.ClusterSettings;
Expand All @@ -27,16 +27,16 @@
public class RestClusterGetSettingsActionTests extends ESTestCase {

public void testFilterPersistentSettings() {
runTestFilterSettingsTest(Metadata.Builder::persistentSettings, ClusterGetSettingsResponse::getPersistentSettings);
runTestFilterSettingsTest(Metadata.Builder::persistentSettings, RestClusterGetSettingsResponse::getPersistentSettings);
}

public void testFilterTransientSettings() {
runTestFilterSettingsTest(Metadata.Builder::transientSettings, ClusterGetSettingsResponse::getTransientSettings);
runTestFilterSettingsTest(Metadata.Builder::transientSettings, RestClusterGetSettingsResponse::getTransientSettings);
}

private void runTestFilterSettingsTest(
final BiConsumer<Metadata.Builder, Settings> md,
final Function<ClusterGetSettingsResponse, Settings> s
final Function<RestClusterGetSettingsResponse, Settings> s
) {
final Metadata.Builder mdBuilder = new Metadata.Builder();
final Settings settings = Settings.builder().put("foo.filtered", "bar").put("foo.non_filtered", "baz").build();
Expand All @@ -52,7 +52,7 @@ private void runTestFilterSettingsTest(
)
).collect(Collectors.toSet());
final ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, settingsSet);
final ClusterGetSettingsResponse response = RestClusterGetSettingsAction.response(
final RestClusterGetSettingsResponse response = RestClusterGetSettingsAction.response(
builder.build(),
randomBoolean(),
filter,
Expand Down

0 comments on commit 5326e7b

Please sign in to comment.