Skip to content

Conversation

@tlrx
Copy link
Member

@tlrx tlrx commented Jul 15, 2019

We improved the CloseIndexResponse in #39687; this pull request exposes it in the HLRC. It also improves the test coverage by using AbstractWireSerializingTestCase and AbstractResponseTestCase.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

This looks good. I left a comment to discuss. Thanks @tlrx.

@SuppressWarnings("unchecked")
private static final ConstructingObjectParser<IndexResult, String> PARSER = new ConstructingObjectParser<>("index_result", true,
(args, name) -> {
final Index index = new Index(name, "_na_");
Copy link
Member

Choose a reason for hiding this comment

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

I think we should avoid using dummy UUIDs in the production code (if possible). However, making IndexResult accept an index name would not be easy now as it will break BWC.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought that we kind of tolerate things like this in the HLRC but my knowledge was just outdated. I talked to @hub-cap about this and he confirmed that we should instead use dedicated HLRC objects.

Thanks for spotting this issue @dnhatn

@tlrx
Copy link
Member Author

tlrx commented Jul 23, 2019

@dnhatn I reverted my changes and I created dedicated objects for the HLRC close index response. This duplicates some logic (mostly in tests) but I think it's cleaner now.

Can you please have another look?

@tlrx tlrx requested a review from dnhatn July 23, 2019 11:56
@tlrx
Copy link
Member Author

tlrx commented Jul 23, 2019

@elasticmachine update branch

@dnhatn
Copy link
Member

dnhatn commented Jul 23, 2019

@elasticmachine update branch

Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the extra iteration.

@tlrx tlrx merged commit a69baf6 into elastic:master Jul 24, 2019
@tlrx tlrx deleted the add-close-index-response-to-hlrc branch July 24, 2019 07:55
@tlrx
Copy link
Member Author

tlrx commented Jul 24, 2019

Thanks a lot @dnhatn !

tlrx added a commit that referenced this pull request Jul 24, 2019
The CloseIndexResponse was improved in #39687; this commit
exposes it in the HLRC.

Backport of #44349 to 7.x.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants