Skip to content
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.

Sign up for GitHub

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

Use chunked encoding for RestGetSettingsAction #90326

Merged
merged 1 commit into from
Sep 27, 2022
Merged

Use chunked encoding for RestGetSettingsAction #90326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 34 additions & 38 deletions ...rc/main/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,11 @@

import org.elasticsearch.action.ActionResponse;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.collect.Iterators;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.ChunkedToXContent;
import org.elasticsearch.common.xcontent.XContentParserUtils;
import org.elasticsearch.xcontent.ToXContent;
import org.elasticsearch.xcontent.ToXContentObject;
Expand All @@ -23,10 +25,11 @@
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
import java.util.Objects;

public class GetSettingsResponse extends ActionResponse implements ToXContentObject {
public class GetSettingsResponse extends ActionResponse implements ToXContentObject, ChunkedToXContent {

private final Map<String, Settings> indexToSettings;
private final Map<String, Settings> indexToDefaultSettings;
Expand All @@ -44,25 +47,12 @@ public GetSettingsResponse(StreamInput in) throws IOException {

/**
* Returns a map of index name to {@link Settings} object. The returned {@link Settings}
* objects contain only those settings explicitly set on a given index. Any settings
* taking effect as defaults must be accessed via {@link #getIndexToDefaultSettings()}.
* objects contain only those settings explicitly set on a given index.
*/
public Map<String, Settings> getIndexToSettings() {
return indexToSettings;
}

/**
* If the originating {@link GetSettingsRequest} object was configured to include
* defaults, this will contain a mapping of index name to {@link Settings} objects.
* The returned {@link Settings} objects will contain only those settings taking
* effect as defaults. Any settings explicitly set on the index will be available
* via {@link #getIndexToSettings()}.
* See also {@link GetSettingsRequest#includeDefaults(boolean)}
*/
public Map<String, Settings> getIndexToDefaultSettings() {
return indexToDefaultSettings;
}

/**
* Returns the string value for the specified index and setting. If the includeDefaults
* flag was not set or set to false on the GetSettingsRequest, this method will only
Expand Down Expand Up @@ -154,38 +144,44 @@ public String toString() {
try {
ByteArrayOutputStream baos = new ByteArrayOutputStream();
XContentBuilder builder = new XContentBuilder(JsonXContent.jsonXContent, baos);
toXContent(builder, ToXContent.EMPTY_PARAMS, false);
var iterator = toXContentChunked(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we think this can grow to 10M+, should we then not avoid assembling this fully here, i.e., limit the output to say 100 entries?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, not entirely trivial to do that, would have to add more logic to achieve that. I wonder if it matters all that much, we have the same problem for the other large responses as well.

I'll try to address this in the next PR on this while dropping the toXContent from these objects also. Incoming :)

while (iterator.hasNext()) {
iterator.next().toXContent(builder, ToXContent.EMPTY_PARAMS);
}
return Strings.toString(builder);
} catch (IOException e) {
throw new IllegalStateException(e); // should not be possible here
}
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
return toXContent(builder, params, indexToDefaultSettings.isEmpty());
public Iterator<? extends ToXContent> toXContentChunked() {
final boolean omitEmptySettings = indexToDefaultSettings.isEmpty();
return toXContentChunked(omitEmptySettings);
}

private XContentBuilder toXContent(XContentBuilder builder, Params params, boolean omitEmptySettings) throws IOException {
builder.startObject();
for (Map.Entry<String, Settings> cursor : getIndexToSettings().entrySet()) {
// no settings, jump over it to shorten the response data
if (omitEmptySettings && cursor.getValue().isEmpty()) {
continue;
}
builder.startObject(cursor.getKey());
builder.startObject("settings");
cursor.getValue().toXContent(builder, params);
builder.endObject();
if (indexToDefaultSettings.isEmpty() == false) {
builder.startObject("defaults");
indexToDefaultSettings.get(cursor.getKey()).toXContent(builder, params);
builder.endObject();
}
builder.endObject();
}
builder.endObject();
return builder;
private Iterator<ToXContent> toXContentChunked(boolean omitEmptySettings) {
final boolean indexToDefaultSettingsEmpty = indexToDefaultSettings.isEmpty();
return Iterators.concat(
Iterators.single((builder, params) -> builder.startObject()),
getIndexToSettings().entrySet()
.stream()
.filter(entry -> omitEmptySettings == false || entry.getValue().isEmpty() == false)
.map(entry -> (ToXContent) (builder, params) -> {
builder.startObject(entry.getKey());
builder.startObject("settings");
entry.getValue().toXContent(builder, params);
builder.endObject();
if (indexToDefaultSettingsEmpty == false) {
builder.startObject("defaults");
indexToDefaultSettings.get(entry.getKey()).toXContent(builder, params);
builder.endObject();
}
return builder.endObject();
})
.iterator(),
Iterators.single((builder, params) -> builder.endObject())
);
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetSettingsAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
import org.elasticsearch.common.Strings;
import org.elasticsearch.rest.BaseRestHandler;
import org.elasticsearch.rest.RestRequest;
import org.elasticsearch.rest.action.RestToXContentListener;
import org.elasticsearch.rest.action.RestChunkedToXContentListener;

import java.io.IOException;
import java.util.List;
Expand Down Expand Up @@ -52,6 +52,6 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
.names(names);
getSettingsRequest.local(request.paramAsBoolean("local", getSettingsRequest.local()));
getSettingsRequest.masterNodeTimeout(request.paramAsTime("master_timeout", getSettingsRequest.masterNodeTimeout()));
return channel -> client.admin().indices().getSettings(getSettingsRequest, new RestToXContentListener<>(channel));
return channel -> client.admin().indices().getSettings(getSettingsRequest, new RestChunkedToXContentListener<>(channel));
}
}
11 changes: 11 additions & 0 deletions ...st/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponseTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,15 @@ protected Predicate<String> getRandomFieldsExcludeFilter() {
return f -> f.equals("") || f.contains(".settings") || f.contains(".defaults");
}

public void testOneChunkPerIndex() {
final var instance = createTestInstance();
final var iterator = instance.toXContentChunked();
int chunks = 0;
while (iterator.hasNext()) {
chunks++;
iterator.next();
}
assertEquals(2 + instance.getIndexToSettings().size(), chunks);
}

}