Change BroadcastResponse from ToXContentFragment to ToXContentObject #28878

PnPie · 2018-03-01T22:29:15Z

While working on #27799, we find that it might make sense to change BroadcastResponse from ToXContentFragment to ToXContentObject, seeing that it's rather a complete XContent object and also the other Responses are normally ToXContentObject.

By doing this, we simplify the Response building in Rest layer by moving the implementation to corresponding response classes themselves, (maybe) where it should be.

Relates to #3889

While working on elastic#27799, we find that it might make sense to change BroadcastResponse from ToXContentFragment to ToXContentObject, seeing that it's rather a complete XContent object and also the other Responses are normally ToXContentObject. By doing this, we can also move the XContent build logic of BroadcastResponse's subclasses, from Rest Layer to the concrete classes themselves.

elasticmachine · 2018-03-01T22:29:17Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine · 2018-03-01T22:29:17Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

olcbean · 2018-03-02T10:16:20Z

server/src/main/java/org/elasticsearch/action/admin/indices/recovery/RecoveryResponse.java

@@ -87,6 +86,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
                if (recoveryStates == null || recoveryStates.size() == 0) {
                    continue;
                }
+                builder.startObject();


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.. )

@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.

javanna

thanks @PnPie I left a couple of comments but this looks good already!

javanna · 2018-03-05T11:51:43Z

server/src/main/java/org/elasticsearch/action/admin/indices/stats/IndicesStatsResponse.java

@@ -197,7 +198,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
            }
            builder.endObject();
        }
-
+        builder.endObject();
        return builder;


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

Of course not :)

javanna · 2018-03-05T11:54:25Z

...er/src/main/java/org/elasticsearch/action/admin/indices/segments/IndicesSegmentResponse.java

@@ -172,6 +174,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
            builder.endObject();
        }

+        builder.endObject();


could you also make the two toXContent static methods private?

javanna · 2018-03-05T13:01:44Z

server/src/main/java/org/elasticsearch/action/admin/indices/recovery/RecoveryResponse.java

@@ -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;


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?

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 😄

I opened #28910 to keep track of this.

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

javanna · 2018-03-05T13:15:56Z

...er/src/main/java/org/elasticsearch/action/admin/indices/segments/IndicesSegmentResponse.java

@@ -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);


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.

javanna · 2018-03-05T13:24:25Z

test this please

javanna · 2018-03-06T10:58:11Z

...c/main/java/org/elasticsearch/action/admin/indices/validate/query/ValidateQueryResponse.java

+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        builder.startObject();
+        builder.field(VALID_FIELD, isValid());
+        RestActions.buildBroadcastShardsHeader(builder, params, this);


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

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 ?

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.

javanna · 2018-03-20T11:37:49Z

retest this please

javanna

hi @PnPie sorry it took me a while to get back to this. I had one last look, ran the tests and found two small issues. Once those are addressed, I can merge this one in. I also took the liberty to merge upstream changes in and push corresponding commits to your repo to ease your work. Thanks!

javanna · 2018-03-20T12:05:22Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestRefreshAction.java

-                return new BytesRestResponse(response.getStatus(), builder);
-            }
-        });
+        return channel -> client.admin().indices().refresh(refreshRequest, new RestToXContentListener<>(channel));


we are losing the status code from refresh response here. You need to override getStatus in RestToXContentListener and take the status from the response.

javanna · 2018-03-20T12:34:30Z

server/src/main/java/org/elasticsearch/action/admin/indices/stats/IndicesStatsResponse.java

@@ -147,15 +145,14 @@ public void writeTo(StreamOutput out) throws IOException {
    }

    @Override
-    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+    protected void addCustomXContentFields(XContentBuilder builder, Params params) throws IOException {


this makes IndicesStatsResponseTests#testInvalidLevel fail. Could you please update the test so that it passes in a valid XContentBuilder rather than null?

PnPie · 2018-03-20T20:55:55Z

Hello @javanna I update this, thanks for pushing the fix ! and also for another PR :)

javanna · 2018-03-22T08:53:16Z

retest this plase

javanna

LGTM thanks @PnPie !

javanna · 2018-03-22T12:11:38Z

retest this please

javanna · 2018-03-22T12:13:51Z

retest this please

Conflicts resolved: server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestForceMergeAction.java

PnPie · 2018-03-22T22:06:34Z

@javanna I merged master in.

javanna · 2018-03-23T09:53:47Z

thanks a lot @PnPie !

…28878) While working on #27799, we find that it might make sense to change BroadcastResponse from ToXContentFragment to ToXContentObject, seeing that it's rather a complete XContent object and also the other Responses are normally ToXContentObject. By doing this, we can also move the XContent build logic of BroadcastResponse's subclasses, from Rest Layer to the concrete classes themselves. Relates to #3889

* es/master: (27 commits) [Docs] Add rank_eval size parameter k (#29218) [DOCS] Remove ignore_z_value parameter link Docs: Update docs/index_.asciidoc (#29172) Docs: Link C++ client lib elasticlient (#28949) [DOCS] Unregister repository instead of deleting it (#29206) Docs: HighLevelRestClient#multiSearch (#29144) Add Z value support to geo_shape Remove type casts in logging in server component (#28807) Change BroadcastResponse from ToXContentFragment to ToXContentObject (#28878) REST : Split `RestUpgradeAction` into two actions (#29124) Add error file docs to important settings Add note to low-level client docs for DNS caching (#29213) Harden periodically check to avoid endless flush loop (#29125) Remove deprecated options for query_string (#29203) REST high-level client: add force merge API (#28896) Remove license information from README.textile (#29198) Decouple more classes from XContentBuilder and make builder strict (#29197) [Docs] Fix missing closing block in cluster/misc.asciidoc RankEvalRequest should implement IndicesRequest (#29188) Use EnumMap in ClusterBlocks (#29112) ...

* es/6.x: (29 commits) [Docs] Add rank_eval size parameter k (#29218) Docs: Update docs/index_.asciidoc (#29172) Docs: Link C++ client lib elasticlient (#28949) Docs: HighLevelRestClient#multiSearch (#29144) [DOCS] Remove ignore_z_value parameter link Add Z value support to geo_shape Change BroadcastResponse from ToXContentFragment to ToXContentObject (#28878) REST : Split `RestUpgradeAction` into two actions (#29124) [DOCS] Unregister repository instead of deleting it (#29206) Remove type casts in logging in server component (#28807) Add error file docs to important settings Add note to low-level client docs for DNS caching (#29213) testShrinkAfterUpgrade should only set mapping.single_type if bwc version > 5.5.0 Harden periodically check to avoid endless flush loop (#29125) REST high-level client: add force merge API (#28896) Remove license information from README.textile (#29198) Decouple more classes from XContentBuilder and make builder strict (#29197) Propagate mapping.single_type setting on shrinked index (#29202) [Docs] Fix missing closing block in cluster/misc.asciidoc RankEvalRequest should implement IndicesRequest (#29188) ...

* master: (40 commits) Do not optimize append-only if seen normal op with higher seqno (elastic#28787) [test] packaging: gradle tasks for groovy tests (elastic#29046) Prune only gc deletes below local checkpoint (elastic#28790) remove testUnassignedShardAndEmptyNodesInRoutingTable elastic#28745: remove extra option in the composite rest tests Fold EngineDiskUtils into Store, for better lock semantics (elastic#29156) Add file permissions checks to precommit task Remove execute mode bit from source files Optimize the composite aggregation for match_all and range queries (elastic#28745) [Docs] Add rank_eval size parameter k (elastic#29218) [DOCS] Remove ignore_z_value parameter link Docs: Update docs/index_.asciidoc (elastic#29172) Docs: Link C++ client lib elasticlient (elastic#28949) [DOCS] Unregister repository instead of deleting it (elastic#29206) Docs: HighLevelRestClient#multiSearch (elastic#29144) Add Z value support to geo_shape Remove type casts in logging in server component (elastic#28807) Change BroadcastResponse from ToXContentFragment to ToXContentObject (elastic#28878) REST : Split `RestUpgradeAction` into two actions (elastic#29124) Add error file docs to important settings ...

javanna self-requested a review March 2, 2018 09:21

javanna added :Core/Infra/REST API REST infrastructure and utilities >enhancement labels Mar 2, 2018

olcbean reviewed Mar 2, 2018

View reviewed changes

change RecoveryResponse XContent building

1413d0c

javanna self-assigned this Mar 5, 2018

javanna approved these changes Mar 5, 2018

View reviewed changes

address comments

3b5a67d

javanna mentioned this pull request Mar 6, 2018

Recovery API supports details param not detailed #28910

Closed

javanna reviewed Mar 6, 2018

View reviewed changes

PnPie and others added 4 commits March 6, 2018 21:30

Remove detailed flag in RecoveryResponse & address ValidateQueryResponse

c24f791

Merge branch 'master' into broadcastresponse_modif

19c5bf8

fix compile errors

24c18f2

fix test compile error

685b94c

javanna requested changes Mar 20, 2018

View reviewed changes

Address comments

e59cc42

javanna approved these changes Mar 22, 2018

View reviewed changes

Merge branch 'master' into broadcastresponse_modif

a9ecd59

javanna added v7.0.0 v6.3.0 labels Mar 22, 2018

Merge branch 'master' into broadcastresponse_modif

20a49cd

Conflicts resolved: server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestForceMergeAction.java

Merge branch 'master' into broadcastresponse_modif

e25a6ac

javanna merged commit 4a8099c into elastic:master Mar 23, 2018

javanna added the backport pending label Mar 23, 2018

javanna removed the backport pending label Mar 23, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change BroadcastResponse from ToXContentFragment to ToXContentObject #28878

Change BroadcastResponse from ToXContentFragment to ToXContentObject #28878

PnPie commented Mar 1, 2018 •

edited by javanna

Loading

elasticmachine commented Mar 1, 2018

elasticmachine commented Mar 1, 2018

olcbean Mar 2, 2018

PnPie Mar 2, 2018

javanna left a comment

javanna Mar 5, 2018

PnPie Mar 5, 2018

javanna Mar 5, 2018

javanna Mar 5, 2018

PnPie Mar 5, 2018

javanna Mar 6, 2018

javanna Mar 6, 2018

javanna Mar 5, 2018

PnPie Mar 5, 2018

javanna commented Mar 5, 2018

javanna Mar 6, 2018

PnPie Mar 6, 2018

javanna Mar 6, 2018

javanna commented Mar 20, 2018

javanna left a comment

javanna Mar 20, 2018

javanna Mar 20, 2018

PnPie commented Mar 20, 2018

javanna commented Mar 22, 2018

javanna left a comment

javanna commented Mar 22, 2018

javanna commented Mar 22, 2018

PnPie commented Mar 22, 2018

javanna commented Mar 23, 2018

Change BroadcastResponse from ToXContentFragment to ToXContentObject #28878

Change BroadcastResponse from ToXContentFragment to ToXContentObject #28878

Conversation

PnPie commented Mar 1, 2018 • edited by javanna Loading

elasticmachine commented Mar 1, 2018

elasticmachine commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna 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

Choose a reason for hiding this comment

javanna commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Mar 20, 2018

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PnPie commented Mar 20, 2018

javanna commented Mar 22, 2018

javanna left a comment

Choose a reason for hiding this comment

javanna commented Mar 22, 2018

javanna commented Mar 22, 2018

PnPie commented Mar 22, 2018

javanna commented Mar 23, 2018

PnPie commented Mar 1, 2018 •

edited by javanna

Loading