Add fromxcontent methods to index response #22229

tlrx · 2016-12-16T14:48:03Z

This commit adds the parsing fromXContent() methods to the IndexResponse class. The method is based on a ObjectParser because it is easier to reuse parsing method from parent abstract classes like DocWriteResponse.

It also changes a bit the toXContent() method of ReplicationResponse.ShardInfo so that it now starts its own object. This way, the ShardInfo.fromXContent() method can be directly referenced by the IndexResponse's ObjectParser.

tlrx · 2016-12-16T14:53:00Z

core/src/test/java/org/elasticsearch/action/support/replication/ReplicationResponseTests.java

No need to review this file, it comes from #22196

tlrx · 2016-12-16T14:53:38Z

core/src/main/java/org/elasticsearch/action/support/replication/ReplicationResponse.java

That's the only bit that changed since #22196

ok, I think I had made you revert this change before, but maybe it's ok because we only print this section out in one place, so the _SHARDS name doesn't get copied to other places. One thing: what does this specific change improve? That ShardInfo outputs a valid object alone?

I'm also curious to understand where the rendering of the field was moved, I don't seem to find it in this PR. What is the outer object that is calling this toXContent Method?

It has been moved to DocWriteResponse.innerToXContent() where

shardInfo.toXContent(builder, params);

has been changed to:

builder.field(_SHARDS, shardInfo);

so that ShardInfo is now a ToXContentObject.

This is important for the ObjectParser used to parse the IndexResponse which can then directly use ShardInfo.fromXContent(p).

tlrx · 2016-12-16T14:53:50Z

core/src/main/java/org/elasticsearch/action/support/replication/ReplicationResponse.java

This change comes from #22196

you also needed to upgrade the fact that you don't expect a field name to start with right? rather a start_object straightaway

tlrx · 2016-12-16T14:55:26Z

Ok I removed the "review" flag because this PR is based on #22196 and brings back/duplicates many changes. I'll rebase it once #22196 is in, it will be easier to review.

javanna

left a few comments, no particular blockers though, looks good.

javanna · 2016-12-16T17:55:28Z

core/src/main/java/org/elasticsearch/action/index/IndexResponse.java

this thing is so un-readable :) sorry I couldn't hold it. Loops and ifs read so much easier to me

javanna · 2016-12-16T17:56:57Z

core/src/main/java/org/elasticsearch/action/support/replication/ReplicationResponse.java

ok, I think I had made you revert this change before, but maybe it's ok because we only print this section out in one place, so the _SHARDS name doesn't get copied to other places. One thing: what does this specific change improve? That ShardInfo outputs a valid object alone?

javanna · 2016-12-16T17:57:21Z

core/src/main/java/org/elasticsearch/action/support/replication/ReplicationResponse.java

you also needed to upgrade the fact that you don't expect a field name to start with right? rather a start_object straightaway

javanna · 2016-12-16T18:00:59Z

core/src/test/java/org/elasticsearch/action/index/IndexResponseTests.java

I think we can avoid repeating IndexResponse in all methods

javanna · 2016-12-16T18:02:18Z

core/src/test/java/org/elasticsearch/action/index/IndexResponseTests.java

please do not use convertToMap, it uses content type auto-detection while we know the content-type upfront. just create a parser and call map against it.

javanna · 2016-12-16T18:04:15Z

core/src/test/java/org/elasticsearch/action/index/IndexResponseTests.java

rather than using a map here, couldn't you call fromXContent again? would that make sense?

javanna · 2016-12-16T18:06:13Z

core/src/main/java/org/elasticsearch/action/DocWriteResponse.java

can you clarify what this method parses?

javanna · 2016-12-16T18:07:24Z

core/src/main/java/org/elasticsearch/action/index/IndexResponse.java

is it correct to do this in IndexResponse? I am a bit confused by who does what. Given the toXContent in IndexResponse that only prints out one field, I was expecting most of the parsing to be in DocWriteResponse. That may have to do again with me not understanding ObjectParser and how it works.

tlrx · 2017-01-10T13:07:30Z

@javanna I updated this pull request with the latest changes in master. I'll be happy if you can have a look again.

Few things to keep in mind:

ShardInfo now implements ToXContentObject: it is required to get it parseable using ConstructingObjectParser in IndexResponse.
IndexResponse uses ObjectParser instead of the "manual" parsing because I find it easier when there is a class hierarchy like IndexResponse->DocWriteResponse

javanna

left few minors, LGTM, @cbuescher want to have a look too? You will need shard failures in search too I think.

javanna · 2017-01-10T13:28:37Z

core/src/main/java/org/elasticsearch/action/index/IndexResponse.java

it is quite odd that the only info we actually can parse back out of the ShardId is the index name. no index_uuid and no shard_id. Maybe we should look into carrying around the index name only as a follow-up in DocWriteResponse. I am not sure we need more, it doesn't seem like it.

Right, that makes sense. I can change that in a follow up PR.

We talked about this with Luca and we both think that we should not do this change. This would impact the serialization of DocWriteResponse in core in order to simplify things and carry less information from a client side point of view.

javanna · 2017-01-10T13:28:59Z

core/src/main/java/org/elasticsearch/action/support/replication/ReplicationResponse.java

thanks for changing this!

javanna · 2017-01-10T13:32:37Z

core/src/test/java/org/elasticsearch/action/index/IndexResponseTests.java

should this randomization be enabled again?

Yes, this is a leftover.

cbuescher · 2017-01-10T14:30:06Z

core/src/test/java/org/elasticsearch/action/index/IndexRequestTests.java

Also just to understand this: this change here only affects the toString() method, not the actual rest response?

cbuescher · 2017-01-10T14:38:11Z

core/src/test/java/org/elasticsearch/action/index/IndexResponseTests.java

Out of curiosity: why does this test the toString() method, the tests seems to indicate its about the xContent?

Oh right - this is because Strings.toString() uses the toXContent method internally. I can replace this by a direct usage of toXContent.

cbuescher · 2017-01-10T14:49:39Z

@tlrx @javanna I also had a look, left a few understanding questions but other than that LGTM. Unfortunately the ReplicationResponse.Failure that is used there is slightly different from the ShardSearchFailure that is used in the SearchResponse. I will take a close look at it but at this point I think although they are quiet similar, we need to add parsing to both of them.

tlrx · 2017-01-10T15:26:26Z

@javanna @cbuescher Thanks!

This commit adds the parsing fromXContent() methods to the IndexResponse class. The method is based on a ObjectParser because it is easier to use when parsing parent abstract classes like DocWriteResponse. It also changes the ReplicationResponse.ShardInfo so that it now implements ToXContentObject. This way, the ShardInfo.fromXContent() method can be used by the IndexResponse's ObjectParser.

This commit adds the parsing fromXContent() methods to the IndexResponse class. It's a pale copy of what has been done in elastic#22229.

Brought by elastic#22229

tlrx added :Java High Level REST Client v5.2.0 v6.0.0-alpha1 labels Dec 16, 2016

tlrx requested a review from javanna December 16, 2016 14:48

tlrx commented Dec 16, 2016

View reviewed changes

tlrx removed the review label Dec 16, 2016

javanna approved these changes Dec 16, 2016

View reviewed changes

tlrx force-pushed the add-fromxcontent-methods-to-IndexResponse branch 2 times, most recently from 0d657e0 to 040ea0f Compare January 10, 2017 13:00

javanna approved these changes Jan 10, 2017

View reviewed changes

tlrx force-pushed the add-fromxcontent-methods-to-IndexResponse branch from 040ea0f to 64663db Compare January 10, 2017 14:23

cbuescher reviewed Jan 10, 2017

View reviewed changes

tlrx added 2 commits January 10, 2017 16:37

Update after Luca review

fdf0830

tlrx force-pushed the add-fromxcontent-methods-to-IndexResponse branch from 64663db to fdf0830 Compare January 10, 2017 15:37

tlrx merged commit 2dcb05f into elastic:master Jan 10, 2017

tlrx deleted the add-fromxcontent-methods-to-IndexResponse branch January 12, 2017 14:01

dadoonet mentioned this pull request Jan 18, 2017

Add fromxcontent methods to delete response #22680

Merged

dadoonet added a commit to dadoonet/elasticsearch that referenced this pull request Jan 19, 2017

Add fromxcontent methods to delete response

718a6b9

This commit adds the parsing fromXContent() methods to the IndexResponse class. It's a pale copy of what has been done in elastic#22229.

dadoonet added a commit to dadoonet/elasticsearch that referenced this pull request Jan 19, 2017

Use now common methods with index/update

0315dcc

Brought by elastic#22229

clintongormley added >enhancement v5.3.0 and removed v5.2.0 labels Jan 24, 2017

Add fromxcontent methods to index response #22229

Add fromxcontent methods to index response #22229

Uh oh!

Conversation

tlrx commented Dec 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlrx commented Dec 16, 2016

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlrx commented Jan 10, 2017

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Jan 10, 2017

Uh oh!

tlrx commented Jan 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants