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.

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

Add parsing methods to BulkItemResponse #22859

Merged
merged 4 commits into from Feb 15, 2017

Conversation

Projects
None yet
4 participants
@tlrx
Copy link
Member

tlrx commented Jan 30, 2017

This commit adds a parsing method to the BulkItemResponse class. In order to do that, the way DocWriteResponses are parsed has to be changed: ConstructingObjectParser/ObjectParser is removed in favor of a simpler (and more readable, I think) way to parse these objects.

DocWriteResponse now provides the parseInnerToXContent() method that can be used by subclasses (IndexResponse, UpdateReponse and DeleteResponse) to parse the current token/field and potentially update a ParsingContext.

The ParsingContext is a simple POJO used to contain parsed values. It can be passed around from one parsing method to another parsing method. This is what is done in IndexResponse: a ParsingContext is created in IndexResponse.fromXContent(), and the parsing context is passed to IndexResponse.parseXContentFields() that parses fields specific to IndexResponse (like "created") and updates the context, delegating to DocWriteResponse.parseInnerToXContent()
the parsing of any other field. Once all XContent is parsed, IndexResponse.fromXContent() uses the method IndexResponse.fromParsingContext() to create the new instance of IndexResponse.

This behavior allow to reuse parsing code among the class hierarchy while keeping the current behavior. It also allows other objects like BulkItemResponse to reuse the same parsing code to parse DocWriteResponses.

Finally, IndexResponseTests, UpdateResponseTests and DeleteResponseTests have been updated to introduce some random shuffling of fields before the XContent is parsed in order to ensure that the parsing code does not rely on field order.

@tlrx tlrx requested review from javanna and cbuescher Jan 30, 2017

@tlrx tlrx force-pushed the tlrx:add-parsing-methods-to-bulk-item-response branch Feb 1, 2017

@clintongormley clintongormley added v5.4.0 and removed v5.3.0 labels Feb 7, 2017

tlrx added some commits Jan 27, 2017

Add parsing methods to BulkItemResponse
This commit adds a parsing method to the BulkItemResponse class. In order to do that, the way DocWriteResponses are parsed has
to be changed: ConstructingObjectParser/ObjectParser is removed in favor of a simpler and more readable way to parse these objects.

DocWriteResponse now provides the parseInnerToXContent() method that can be used by subclasses (IndexResponse, UpdateReponse and
DeleteResponse) to parse the current token/field and potentially update a ParsingContext. The ParsingContext is a simple POJO used
to contain parsed values. It can be passed around from one parsing method to another parsing method. For example, this is what is done in
IndexResponse: a ParsingContext is created in IndexResponse.fromXContent(), it get passed to IndexResponse.parseXContentFields() that
parses fields specific to IndexResponse (like "created") and updates the context, delegating to DocWriteResponse.parseInnerToXContent()
the parsing of any other field. Once all XContent is parsed, IndexResponse.fromXContent() uses the method
IndexResponse.fromParsingContext() to create the new instance of IndexResponse.

This behavior allow to reuse parsing code among the class hierarchy while keeping the current behavior. It also allows other objects
like BulkItemResponse to reuse the same parsing code to parse DocWriteResponses.

Finally, IndexResponseTests, UpdateResponseTests and DeleteResponseTests have been updated to introduce some random shuffling of
fields before the XContent is parsed in order to ensure that the parsing code does not rely on field order.

@tlrx tlrx force-pushed the tlrx:add-parsing-methods-to-bulk-item-response branch to 8dea9ff Feb 8, 2017

@tlrx

This comment has been minimized.

Copy link
Member Author

tlrx commented Feb 8, 2017

Rebased to resolve conficts.

@cbuescher
Copy link
Member

cbuescher left a comment

@tlrx thanks, I think I see the problem why reusing the current ObjectParsers in Index/Delete/UpdateResponse is not possible. I think the solution is good, even if it took me some time to wrap my head around. Leaving a few questions and comments for now.

objParser.declareObject(optionalConstructorArg(), (p, c) -> ShardInfo.fromXContent(p), new ParseField(_SHARDS));
objParser.declareLong(optionalConstructorArg(), new ParseField(_SEQ_NO));
objParser.declareBoolean(DocWriteResponse::setForcedRefresh, new ParseField(FORCED_REFRESH));
public static void parseInnerToXContent(XContentParser parser, ParsingContext context) throws IOException {

This comment has been minimized.

Copy link
@cbuescher

cbuescher Feb 10, 2017

Member

I think this could be protected, since this is only ever called by subclasses.

/**
* {@link ParsingContext} holds information about {@link DocWriteResponse} objects during XContent parsing.
*/
public static class ParsingContext {

This comment has been minimized.

Copy link
@cbuescher

cbuescher Feb 10, 2017

Member

Can be abstract? I think only the three concrete implementations are supposed to be instantiated

This comment has been minimized.

Copy link
@cbuescher

cbuescher Feb 10, 2017

Member

Also my 2 cents about naming here: I think ParsingContext is a bit broad, with context I usually associate something that carries around state or other dependencies of the method it is used in. To me this is more like a builder or a temporary object. Not really sure about a better name atm. Maybe call the abstract one DocWriteResponseBuilder and the specializations according to their classes?

This comment has been minimized.

Copy link
@javanna

javanna Feb 10, 2017

Member

I tend to agree that naming could be improved, e.g. I would be ok with builder

This comment has been minimized.

Copy link
@tlrx

tlrx Feb 13, 2017

Author Member

Sure, thanks for the suggestions @cbuescher

private Long seqNo = SequenceNumbersService.UNASSIGNED_SEQ_NO;

public ParsingContext(){
}

This comment has been minimized.

Copy link
@cbuescher

cbuescher Feb 10, 2017

Member

Nit: ctor not really needed?

/**
* Create a {@link DeleteResponse} from a parsing context.
*/
public static DeleteResponse fromParsingContext(ParsingContext context) {

This comment has been minimized.

Copy link
@cbuescher

cbuescher Feb 10, 2017

Member

I wonder if all these fromParsingContext methods could rather be build methods in the respective ParsingContext objects (they don't have to be called build). That way each "specialized" ParsingContext could accumulate all the info needed during the parsing and the output the right Object in the end. It would probably also enable restricting access (e.g. removing most of the getters from the ParsingContext).

This comment has been minimized.

Copy link
@tlrx

tlrx Feb 13, 2017

Author Member

That's a very good idea

ensureExpectedToken(XContentParser.Token.END_OBJECT, token, parser::getTokenLocation);

// Id cannot be parsed at this level
int id = -1;

This comment has been minimized.

Copy link
@cbuescher

cbuescher Feb 10, 2017

Member

Just out of interest: Could we later set the id on the side where this fromXContent() is called? It seems to be the position of the item in the bulk response? Maybe this could be passed in as a second argument to the XContentParser?

This comment has been minimized.

Copy link
@tlrx

tlrx Feb 13, 2017

Author Member

Yes it should be passed as an argument, I changed that, thanks.

assertEquals(opType.getLowercase(), parser.currentName());
assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken());

Map<String, Object> map = parser.map();

This comment has been minimized.

Copy link
@cbuescher

cbuescher Feb 10, 2017

Member

What was the reason again why we cannot simply compare the parsed objects xContent representation to the original? I forgot, seems to make this test a bit more complicated than I thought.

This comment has been minimized.

Copy link
@tlrx

tlrx Feb 13, 2017

Author Member

The reason is that DocWriteResponse are generated in a randomized fashion and can have ShardInfo.Failure. These failures can contain exceptions, which are not directly comparable (because exception lose their types when they are parsed back).

I agree it's complicated in the tests. I don't like the actual map and all DocWriteResponse tests should provide a Tuple<IndexReponse, IndexResponse> createTestItem() method that provides the object to serialize and the expected result. It is not the case right now, and the BulkItemResponseTests just reuse the current methods from others testing class.

Maybe it could go as it is for now using what other tests already use, and I can clean this up in a follow up PR?

This comment has been minimized.

Copy link
@cbuescher

cbuescher Feb 15, 2017

Member

Okay, we agreed to do this a a follow-up PR.

This comment has been minimized.

Copy link
@tlrx

tlrx Feb 17, 2017

Author Member

Follow-up is #23233

@tlrx

This comment has been minimized.

Copy link
Member Author

tlrx commented Feb 13, 2017

Thanks @cbuescher, very useful review! I addressed most of the comments, please let me know what you think.

@cbuescher
Copy link
Member

cbuescher left a comment

@tlrx thanks, this looks great. I left a couple of minor comments but nothing that should hold up merging this. Feel free to address as you like.


DocWriteResponse.DocWriteResponseBuilder builder = null;
CheckedConsumer<XContentParser, IOException> itemParser = null;
Supplier<? extends DocWriteResponse> itemSupplier = null;

This comment has been minimized.

Copy link
@cbuescher

cbuescher Feb 15, 2017

Member

Nit: I think you can remove the itemSupplier and call builder.build() instead later.

This comment has been minimized.

Copy link
@tlrx

tlrx Feb 15, 2017

Author Member

Sure, thanks

static final String STATUS = "status";
static final String ERROR = "error";
public static BulkItemResponse fromXContent(XContentParser parser, int id) throws IOException {
XContentParser.Token token = parser.nextToken();

This comment has been minimized.

Copy link
@cbuescher

cbuescher Feb 15, 2017

Member

Nit: in other places advancing the parser is directly done in the "ensureExpectedToken" call. Saves one line and I think it is equally readable. There's several places in this method where this might be possible, not sure how much lines this saves. Also, if you don't like it, just a matter of tase I think

This comment has been minimized.

Copy link
@tlrx

tlrx Feb 15, 2017

Author Member

Ok


if (STATUS.equals(currentFieldName)) {
// ignoring status field
continue;

This comment has been minimized.

Copy link
@cbuescher

cbuescher Feb 15, 2017

Member

Nit: it would be great if the loop structure could be changed somehow so these "continue" statements wouldn't be necessary. Not sure if this complicates stuff though. Also I think explicitely consuming the status field value here would improve readability a little bit.

static final String _ID = "_id";
static final String STATUS = "status";
static final String ERROR = "error";
public static BulkItemResponse fromXContent(XContentParser parser, int id) throws IOException {

This comment has been minimized.

Copy link
@cbuescher

cbuescher Feb 15, 2017

Member

Can you add a comment explaining that the id? As far as I see it its the bulk item ordering number but would be good to have this info around for future reference.

This comment has been minimized.

Copy link
@tlrx

tlrx Feb 15, 2017

Author Member

Sure

originalBytes = toXContent(new BulkItemResponse(bulkItemId, opType, updates.v1()), xContentType, humanReadable);

} else {
fail("Test does not support opType [" + opType + "]");

This comment has been minimized.

Copy link
@cbuescher

cbuescher Feb 15, 2017

Member

Is this is necessary given we loop over OpType.values()? If other values are added in the future we should fail because expectedBulkItemResponse/originalBytes is not set anyway.

This comment has been minimized.

Copy link
@tlrx

tlrx Feb 15, 2017

Author Member

Right, we can remove this

assertEquals(opType.getLowercase(), parser.currentName());
assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken());

Map<String, Object> map = parser.map();

This comment has been minimized.

Copy link
@cbuescher

cbuescher Feb 15, 2017

Member

Okay, we agreed to do this a a follow-up PR.

@tlrx tlrx merged commit e8d669f into elastic:master Feb 15, 2017

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details
@tlrx

This comment has been minimized.

Copy link
Member Author

tlrx commented Feb 15, 2017

Thanks @cbuescher, I merged and implemented your latest suggestions.

@tlrx tlrx deleted the tlrx:add-parsing-methods-to-bulk-item-response branch Feb 15, 2017

tlrx added a commit that referenced this pull request Feb 16, 2017

Add parsing methods to BulkItemResponse (#22859)
This commit adds a parsing method to the BulkItemResponse class. In order to do that, the way DocWriteResponses are parsed has to be changed: ConstructingObjectParser/ObjectParser is removed in favor of a simpler and more readable way to parse these objects.

DocWriteResponse now provides the parseInnerToXContent() method that can be used by subclasses (IndexResponse, UpdateReponse and DeleteResponse) to parse the current token/field and potentially update a DocWriteResponseBuilder. The DocWriteResponseBuilder is a simple POJO used
to contain parsed values. It can be passed around from one parsing method to another parsing method. For example, this is what is done in IndexResponse: a IndexResponseBuilder is created in IndexResponse.fromXContent(), it get passed to IndexResponse.parseXContentFields() that
parses fields specific to IndexResponse (like "created") and updates the context, delegating to DocWriteResponse.parseInnerToXContent() the parsing of any other field. Once all XContent is parsed, IndexResponse.fromXContent() uses the method
IndexResponseBuilder.build() to create the new instance of IndexResponse.

This behavior allow to reuse parsing code among the class hierarchy while keeping the current behavior. It also allows other objects like BulkItemResponse to reuse the same parsing code to parse DocWriteResponses.

Finally, IndexResponseTests, UpdateResponseTests and DeleteResponseTests have been updated to introduce some random shuffling of fields before the XContent is parsed in order to ensure that the parsing code does not rely on field order.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.