Skip to content

Change BroadcastResponse from ToXContentFragment to ToXContentObject #28878

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

Merged
merged 11 commits into from
Mar 23, 2018
Merged

Change BroadcastResponse from ToXContentFragment to ToXContentObject #28878

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
826c79c
Change BroadcastResponse from ToXContentFragment to ToXContentObject
PnPie Mar 1, 2018
1413d0c
change RecoveryResponse XContent building
PnPie Mar 2, 2018
3b5a67d
address comments
PnPie Mar 5, 2018
c24f791
Remove detailed flag in RecoveryResponse & address ValidateQueryResponse
PnPie Mar 6, 2018
19c5bf8
Merge branch 'master' into broadcastresponse_modif
javanna Mar 20, 2018
24c18f2
fix compile errors
javanna Mar 20, 2018
685b94c
fix test compile error
javanna Mar 20, 2018
e59cc42
Address comments
PnPie Mar 20, 2018
a9ecd59
Merge branch 'master' into broadcastresponse_modif
javanna Mar 22, 2018
20a49cd
Merge branch 'master' into broadcastresponse_modif
PnPie Mar 22, 2018
e25a6ac
Merge branch 'master' into broadcastresponse_modif
javanna Mar 23, 2018
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
7 changes: 4 additions & 3 deletions server/src/main/java/org/elasticsearch/action/admin/indices/recovery/RecoveryResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.ToXContentFragment;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.indices.recovery.RecoveryState;

Expand All @@ -37,7 +36,7 @@
/**
* Information regarding the recovery state of indices and their associated shards.
*/
public class RecoveryResponse extends BroadcastResponse implements ToXContentFragment {
public class RecoveryResponse extends BroadcastResponse {

private boolean detailed = false;
Copy link
Member

Choose a reason for hiding this comment

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

I just figured this flag is never read, we can rather remove it and simplify the rest action. We should also remove it from the request as it does nothing, and adjust the docs. The correct parameter is details and not detailed, but we don't accept it anymore since we introduced params validation a while ago. The flag doesn't need to be serialized either, as it only affects the xcontent output, is is simply a REST param passed through to the response like we do in other places. Would you like to open a follow-up PR to fix this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, cool ! of course :) I'll look at it after finishing these PRs, and it's like this it changes a little whlie working on Add Broadcast API 😄

Copy link
Member

Choose a reason for hiding this comment

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

I opened #28910 to keep track of this.

Copy link
Member

Choose a reason for hiding this comment

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

what do you think of removing the flag already from the response given that it does nothing? I think it would simplify the rest action

private Map<String, List<RecoveryState>> shardRecoveryStates = new HashMap<>();
Expand Down Expand Up @@ -87,6 +86,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
if (recoveryStates == null || recoveryStates.size() == 0) {
continue;
}
builder.startObject();
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this result in an empty builder when !hasRecoveries()? Shouldn't we be returning an empty object instead?
( Maybe it is ok, I don't what the convention is.. )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@olcbean Yes, you are right. The builder.startObject(); builder.endObject(); pair should be put outside the condition I think. Just like what it was previously.

builder.startObject(index);
builder.startArray("shards");
for (RecoveryState recoveryState : recoveryStates) {
Expand All @@ -96,6 +96,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
}
builder.endArray();
builder.endObject();
builder.endObject();
}
}
return builder;
Expand Down Expand Up @@ -133,4 +134,4 @@ public void readFrom(StreamInput in) throws IOException {
public String toString() {
return Strings.toString(this, true, true);
}
}
}
7 changes: 5 additions & 2 deletions ...src/main/java/org/elasticsearch/action/admin/indices/segments/IndicesSegmentResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.xcontent.ToXContentFragment;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.index.engine.Segment;
import org.elasticsearch.rest.action.RestActions;

import java.io.IOException;
import java.util.ArrayList;
Expand All @@ -43,7 +43,7 @@
import java.util.Map;
import java.util.Set;

public class IndicesSegmentResponse extends BroadcastResponse implements ToXContentFragment {
public class IndicesSegmentResponse extends BroadcastResponse {

private ShardSegments[] shards;

Expand Down Expand Up @@ -104,6 +104,8 @@ public void writeTo(StreamOutput out) throws IOException {

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
RestActions.buildBroadcastShardsHeader(builder, params, this);
Copy link
Member

Choose a reason for hiding this comment

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

given that in most cases we have the shards header, how about doing this in BroadcastResponse:

public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
    builder.startObject();
    RestActions.buildBroadcastShardsHeader(builder, params, this);
    addCustomFields();
    builder.endObject();
}

protected void addCustomFields() {

}

subclasses can override this additional method to add their own fields rather than repeating the shards header every time. There is always the possibility to also override toXContent for content where the shards header is not printed out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly !

builder.startObject(Fields.INDICES);

for (IndexSegments indexSegments : getIndices().values()) {
Expand Down Expand Up @@ -172,6 +174,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
builder.endObject();
}

builder.endObject();
Copy link
Member

Choose a reason for hiding this comment

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

could you also make the two toXContent static methods private?

builder.endObject();
return builder;
}
Expand Down
9 changes: 5 additions & 4 deletions server/src/main/java/org/elasticsearch/action/admin/indices/stats/IndicesStatsResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@
import org.elasticsearch.cluster.routing.ShardRouting;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.ToXContentFragment;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.rest.action.RestActions;

import java.io.IOException;
import java.util.ArrayList;
Expand All @@ -38,7 +38,7 @@

import static java.util.Collections.unmodifiableMap;

public class IndicesStatsResponse extends BroadcastResponse implements ToXContentFragment {
public class IndicesStatsResponse extends BroadcastResponse {

private ShardStats[] shards;

Expand Down Expand Up @@ -154,7 +154,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
throw new IllegalArgumentException("level parameter must be one of [cluster] or [indices] or [shards] but was [" + level + "]");
}


builder.startObject();
RestActions.buildBroadcastShardsHeader(builder, params, this);
builder.startObject("_all");

builder.startObject("primaries");
Expand Down Expand Up @@ -197,7 +198,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
}
builder.endObject();
}

builder.endObject();
return builder;
Copy link
Member

Choose a reason for hiding this comment

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

would you mind also changing the toString method here to call Strings.toString instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course not :)

}

Expand Down
5 changes: 3 additions & 2 deletions ...c/main/java/org/elasticsearch/action/admin/indices/upgrade/get/UpgradeStatusResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import org.elasticsearch.action.support.broadcast.BroadcastResponse;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.ToXContentFragment;
import org.elasticsearch.common.xcontent.XContentBuilder;

import java.io.IOException;
Expand All @@ -34,7 +33,7 @@
import java.util.Map;
import java.util.Set;

public class UpgradeStatusResponse extends BroadcastResponse implements ToXContentFragment {
public class UpgradeStatusResponse extends BroadcastResponse {
private ShardUpgradeStatus[] shards;

private Map<String, IndexUpgradeStatus> indicesUpgradeStatus;
Expand Down Expand Up @@ -116,6 +115,7 @@ public long getToUpgradeBytesAncient() {

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
builder.byteSizeField(Fields.SIZE_IN_BYTES, Fields.SIZE, getTotalBytes());
builder.byteSizeField(Fields.SIZE_TO_UPGRADE_IN_BYTES, Fields.SIZE_TO_UPGRADE, getToUpgradeBytes());
builder.byteSizeField(Fields.SIZE_TO_UPGRADE_ANCIENT_IN_BYTES, Fields.SIZE_TO_UPGRADE_ANCIENT, getToUpgradeBytesAncient());
Expand Down Expand Up @@ -161,6 +161,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
}
builder.endObject();
}
builder.endObject();
return builder;
}

Expand Down
18 changes: 18 additions & 0 deletions ...er/src/main/java/org/elasticsearch/action/admin/indices/upgrade/post/UpgradeResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import org.elasticsearch.common.collect.Tuple;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.rest.action.RestActions;

import java.io.IOException;
import java.util.HashMap;
Expand Down Expand Up @@ -74,6 +76,22 @@ public void writeTo(StreamOutput out) throws IOException {
}
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
RestActions.buildBroadcastShardsHeader(builder, params, this);
builder.startObject("upgraded_indices");
for (Map.Entry<String, Tuple<Version, String>> entry : versions.entrySet()) {
builder.startObject(entry.getKey());
builder.field("upgrade_version", entry.getValue().v1());
builder.field("oldest_lucene_segment_version", entry.getValue().v2());
builder.endObject();
}
builder.endObject();
builder.endObject();
return builder;
}

/**
* Returns the highest upgrade version of the node that performed metadata upgrade and the
* the version of the oldest lucene segment for each index that was upgraded.
Expand Down
41 changes: 40 additions & 1 deletion ...ain/java/org/elasticsearch/action/admin/indices/validate/query/ValidateQueryResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import org.elasticsearch.action.support.broadcast.BroadcastResponse;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.rest.action.RestActions;

import java.io.IOException;
import java.util.ArrayList;
Expand All @@ -38,8 +40,15 @@
*/
public class ValidateQueryResponse extends BroadcastResponse {

public static final String INDEX_FIELD = "index";
public static final String SHARD_FIELD = "shard";
public static final String VALID_FIELD = "valid";
public static final String EXPLANATIONS_FIELD = "explanations";
public static final String ERROR_FIELD = "error";
public static final String EXPLANATION_FIELD = "explanation";

private boolean valid;

private List<QueryExplanation> queryExplanations;

ValidateQueryResponse() {
Expand Down Expand Up @@ -96,4 +105,34 @@ public void writeTo(StreamOutput out) throws IOException {
}

}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
builder.field(VALID_FIELD, isValid());
RestActions.buildBroadcastShardsHeader(builder, params, this);
Copy link
Member

Choose a reason for hiding this comment

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

shall we leverage addCustomFields here too and just move the valid field after the shards header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like we are putting the validate field at the beginning on purpose https://www.elastic.co/guide/en/elasticsearch/reference/current/search-validate.html ? as it's for validation and we want to see the validate field at first ? If we modify the order here, will it impact elsewhere when it calls toXContent() of ValidateQueryResponse ?

Copy link
Member

Choose a reason for hiding this comment

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

I see, but you can't rely on keys ordering when parsing json... we are free to change order as that is not guaranteed. Maybe we should then update the docs as well accordingly just to make sure that nobody thinks we guarantee that valid comes first.

if (getQueryExplanation() != null && !getQueryExplanation().isEmpty()) {
builder.startArray(EXPLANATIONS_FIELD);
for (QueryExplanation explanation : getQueryExplanation()) {
builder.startObject();
if (explanation.getIndex() != null) {
builder.field(INDEX_FIELD, explanation.getIndex());
}
if(explanation.getShard() >= 0) {
builder.field(SHARD_FIELD, explanation.getShard());
}
builder.field(VALID_FIELD, explanation.isValid());
if (explanation.getError() != null) {
builder.field(ERROR_FIELD, explanation.getError());
}
if (explanation.getExplanation() != null) {
builder.field(EXPLANATION_FIELD, explanation.getExplanation());
}
builder.endObject();
}
builder.endArray();
}
builder.endObject();
return builder;
}
}
6 changes: 4 additions & 2 deletions server/src/main/java/org/elasticsearch/action/support/broadcast/BroadcastResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
import org.elasticsearch.common.xcontent.ToXContentFragment;
import org.elasticsearch.common.xcontent.ToXContentObject;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.rest.action.RestActions;
Expand All @@ -40,7 +40,7 @@
/**
* Base class for all broadcast operation based responses.
*/
public class BroadcastResponse extends ActionResponse implements ToXContentFragment {
public class BroadcastResponse extends ActionResponse implements ToXContentObject {

public static final DefaultShardOperationFailedException[] EMPTY = new DefaultShardOperationFailedException[0];

Expand Down Expand Up @@ -149,7 +149,9 @@ public void writeTo(StreamOutput out) throws IOException {

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
RestActions.buildBroadcastShardsHeader(builder, params, this);
builder.endObject();
return builder;
}
}
19 changes: 2 additions & 17 deletions ...rc/main/java/org/elasticsearch/rest/action/admin/indices/RestClearIndicesCacheAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,28 +20,22 @@
package org.elasticsearch.rest.action.admin.indices;

import org.elasticsearch.action.admin.indices.cache.clear.ClearIndicesCacheRequest;
import org.elasticsearch.action.admin.indices.cache.clear.ClearIndicesCacheResponse;
import org.elasticsearch.action.support.IndicesOptions;
import org.elasticsearch.client.node.NodeClient;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.rest.BaseRestHandler;
import org.elasticsearch.rest.BytesRestResponse;
import org.elasticsearch.rest.RestController;
import org.elasticsearch.rest.RestRequest;
import org.elasticsearch.rest.RestResponse;
import org.elasticsearch.rest.action.RestBuilderListener;
import org.elasticsearch.rest.action.RestToXContentListener;

import java.io.IOException;
import java.util.Map;

import static org.elasticsearch.rest.RestRequest.Method.GET;
import static org.elasticsearch.rest.RestRequest.Method.POST;
import static org.elasticsearch.rest.RestStatus.OK;
import static org.elasticsearch.rest.action.RestActions.buildBroadcastShardsHeader;

public class RestClearIndicesCacheAction extends BaseRestHandler {
public RestClearIndicesCacheAction(Settings settings, RestController controller) {
Expand All @@ -64,16 +58,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
Strings.splitStringByCommaToArray(request.param("index")));
clearIndicesCacheRequest.indicesOptions(IndicesOptions.fromRequest(request, clearIndicesCacheRequest.indicesOptions()));
fromRequest(request, clearIndicesCacheRequest);
return channel ->
client.admin().indices().clearCache(clearIndicesCacheRequest, new RestBuilderListener<ClearIndicesCacheResponse>(channel) {
@Override
public RestResponse buildResponse(ClearIndicesCacheResponse response, XContentBuilder builder) throws Exception {
builder.startObject();
buildBroadcastShardsHeader(builder, request, response);
builder.endObject();
return new BytesRestResponse(OK, builder);
}
});
return channel -> client.admin().indices().clearCache(clearIndicesCacheRequest, new RestToXContentListener<>(channel));
}

@Override
Expand Down
17 changes: 2 additions & 15 deletions server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestFlushAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,24 +20,19 @@
package org.elasticsearch.rest.action.admin.indices;

import org.elasticsearch.action.admin.indices.flush.FlushRequest;
import org.elasticsearch.action.admin.indices.flush.FlushResponse;
import org.elasticsearch.action.support.IndicesOptions;
import org.elasticsearch.client.node.NodeClient;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.rest.BaseRestHandler;
import org.elasticsearch.rest.BytesRestResponse;
import org.elasticsearch.rest.RestController;
import org.elasticsearch.rest.RestRequest;
import org.elasticsearch.rest.RestResponse;
import org.elasticsearch.rest.action.RestBuilderListener;
import org.elasticsearch.rest.action.RestToXContentListener;

import java.io.IOException;

import static org.elasticsearch.rest.RestRequest.Method.GET;
import static org.elasticsearch.rest.RestRequest.Method.POST;
import static org.elasticsearch.rest.RestStatus.OK;

public class RestFlushAction extends BaseRestHandler {
public RestFlushAction(Settings settings, RestController controller) {
Expand All @@ -60,14 +55,6 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
flushRequest.indicesOptions(IndicesOptions.fromRequest(request, flushRequest.indicesOptions()));
flushRequest.force(request.paramAsBoolean("force", flushRequest.force()));
flushRequest.waitIfOngoing(request.paramAsBoolean("wait_if_ongoing", flushRequest.waitIfOngoing()));
return channel -> client.admin().indices().flush(flushRequest, new RestBuilderListener<FlushResponse>(channel) {
@Override
public RestResponse buildResponse(FlushResponse response, XContentBuilder builder) throws Exception {
builder.startObject();
response.toXContent(builder, request);
builder.endObject();
return new BytesRestResponse(OK, builder);
}
});
return channel -> client.admin().indices().flush(flushRequest, new RestToXContentListener<>(channel));
}
}
11 changes: 2 additions & 9 deletions server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestForceMergeAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.elasticsearch.rest.RestRequest;
import org.elasticsearch.rest.RestResponse;
import org.elasticsearch.rest.action.RestBuilderListener;
import org.elasticsearch.rest.action.RestToXContentListener;

import java.io.IOException;

Expand All @@ -58,14 +59,6 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
mergeRequest.maxNumSegments(request.paramAsInt("max_num_segments", mergeRequest.maxNumSegments()));
mergeRequest.onlyExpungeDeletes(request.paramAsBoolean("only_expunge_deletes", mergeRequest.onlyExpungeDeletes()));
mergeRequest.flush(request.paramAsBoolean("flush", mergeRequest.flush()));
return channel -> client.admin().indices().forceMerge(mergeRequest, new RestBuilderListener<ForceMergeResponse>(channel) {
@Override
public RestResponse buildResponse(ForceMergeResponse response, XContentBuilder builder) throws Exception {
builder.startObject();
buildBroadcastShardsHeader(builder, request, response);
builder.endObject();
return new BytesRestResponse(OK, builder);
}
});
return channel -> client.admin().indices().forceMerge(mergeRequest, new RestToXContentListener<>(channel));
}
}
Loading