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

Update IndexTemplateMetaData to allow unknown fields #38448

Merged
merged 3 commits into from
Feb 6, 2019

Conversation

martijnvg
Copy link
Member

Update IndexTemplateMetaData to use ObjectParser
The IndexTemplateMetaData class used old parsing logic and was not
resiliant to new fields. This commit updates it to use the
ConstructingObjectParser and allow unknown fields.

Also a test was added that randomly generates a response that an ES node would returns and
this test verifies that HLRC can parse it. This gives us the test coverage and confidence that
new parsing code is working well.

Relates #36938

hub-cap and others added 2 commits February 4, 2019 14:21
The IndexTemplateMetaData class used old parsing logic and was not
resiliant to new fields. This commit updates it to use the
ConstructingObjectParser and allow unknown fields.

Relates elastic#36938
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

Copy link
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

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

LGTM but i wrote 1/2 the code so id like an independent set of 👓

Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM

@jakelandis
Copy link
Contributor

jakelandis commented Feb 5, 2019

I don't think this can go back to 6.7 , since if I read this correctly... it also removes support for the deprecated "template" field, which Logstash (and possibly others) still uses on 6.x. Also we should probably add this as a breaking change and add to deprecation info API.

EDIT: this is client only code, so we don't need to add to deprecation info API

Copy link
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

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

LGTM for 7.x only

However, I would encourage a test that explicitly tests that "template" is no longer supported.

I just realized this is only for the client code (not server side), so no need to add to breaking changes for server. I am OK with the client being more aggressive here then the server w/r/t to support for the deprecated field. (A test here would help to communicate that too).

@jakelandis
Copy link
Contributor

@martijnvg - if you merge this is 7.x, could you please log a follow up issue to remove support for "template" in the server side parsers, targeted at 8.0 ?

@martijnvg
Copy link
Member Author

@jakelandis The IndexTemplateMetaData.Builder#toInnerXContent(...) method never serializes the index patterns under the pattern key, always under the index_patterns key. Since what this method generates, is what the hrlc will parse, I think this PR can be backported to 6.7? I'm not saying we should, but there is no harm in backporting it?

The logic that with the pattern key is only used for reading xcontent from the client on the server side. This parsing code was copied to hlrc, but can safely be removed.

if you merge this is 7.x, could you please log a follow up issue to remove support for "template" in the server side parsers, targeted at 8.0 ?

Yes, I will open an issue for this.

@jakelandis
Copy link
Contributor

@martijnvg - I see now, I missed this part: "IndexTemplateMetaData.Builder#toInnerXContent(...) method never serializes..." Thanks for explaining.

LGTM for 6.x too.

@martijnvg martijnvg merged commit 9492dce into elastic:master Feb 6, 2019
martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Feb 6, 2019
Updated IndexTemplateMetaData to use ObjectParser.

The IndexTemplateMetaData class used old parsing logic and was not
resiliant to new fields. This commit updates it to use the
ConstructingObjectParser and allow unknown fields.

Relates elastic#36938

Co-authored-by: Michael Basnight <[email protected]>
Co-authored-by: Martijn van Groningen <[email protected]>
martijnvg added a commit that referenced this pull request Feb 6, 2019
Updated IndexTemplateMetaData to use ObjectParser.

The IndexTemplateMetaData class used old parsing logic and was not
resiliant to new fields. This commit updates it to use the
ConstructingObjectParser and allow unknown fields.

Relates #36938

Co-authored-by: Michael Basnight <[email protected]>
Co-authored-by: Martijn van Groningen <[email protected]>
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 6, 2019
…on-leases-recovery

* elastic/master:
  SQL: Allow look-ahead resolution of aliases for WHERE clause (elastic#38450)
  Add API key settings documentation (elastic#38490)
  Remove support for maxRetryTimeout from low-level REST client (elastic#38085)
  Update IndexTemplateMetaData to allow unknown fields (elastic#38448)
  Mute testRetentionLeasesSyncOnRecovery (elastic#38488)
  Change the min supported version to 6.7.0 for API keys (elastic#38481)
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.

6 participants