Skip to content

Support 'include_type_name' in RestGetIndicesAction #37267

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Jan 11, 2019
Merged

Support 'include_type_name' in RestGetIndicesAction #37267

merged 12 commits into from
Jan 11, 2019

Conversation

@cbuescher
Copy link
Member

@cbuescher cbuescher commented Jan 9, 2019

This is the backport of #37149, introducing the "include_type_name" parameter
to the indices.get API. The difference is that the parameter defaults to "true" on 6.x,
leading to the old known response format. A deprecation is issues when the parameter
isn't used or is set to "true" (the default) to make users aware of the changed default
behaviour starting in 7.0 and moving them towards the new typeless response format.

Christoph Büscher added 3 commits January 9, 2019 14:36
Support 'include_type_name' in RestGetIndicesAction (elastic#37149)
ebfcf07 
This change adds support for the 'include_type_name' parameter for the
indices.get API. This parameter, which defaults to `false` starting in 7.0,
changes the response to not include the indices type names any longer.

If the parameter is set in the request, we additionally emit a deprecation
warning since using the parameter should be only temporarily necessary while
adapting to the new response format and we will remove it with the next major
version.
Fix type inference related compile issue in Eclipse
05e40cd 
Help Eclipse compiler infering correct type argumentes by giving some explicit
casting hints.
iter
2816544
@cbuescher cbuescher added :Search Foundations/Mapping Index mappings, including merging and defining field types backport v6.7.0 labels Jan 9, 2019
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jan 9, 2019

Pinging @elastic/es-search

@cbuescher cbuescher requested a review from jtibshirani January 9, 2019 15:09
jtibshirani
jtibshirani reviewed Jan 9, 2019
View reviewed changes
Copy link
Contributor

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

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

This is looking good to me, I just left some small comments.

@jtibshirani
Copy link
Contributor

jtibshirani commented Jan 9, 2019

One other comment I had forgotten -- should we start explicitly setting include_type_name = true now in the Java HLRC, to make sure that the 6.7 client is compatible with 7.0, which has a different default for include_type_name?

@cbuescher
Copy link
Member Author

cbuescher commented Jan 10, 2019

Should we start explicitly setting include_type_name = true now in the Java HLRC, to make sure that the 6.7 client is compatible with 7.0

This should already happen by using the default value true if no parameter is set on the request. By testing this we should also catch accidental changes to the default, since the HLRC client will not be able to parse the response. I think that should be enough, no?

@jtibshirani
Copy link
Contributor

jtibshirani commented Jan 10, 2019

This should already happen by using the default value true if no parameter is set on the request.

We would like the 6.7 HLRC to be able to understand responses from 7.0 nodes. Since include_type_name will default to false in 7.0, we therefore need to explicitly set include_type_name=true when creating the HLRC request.

jtibshirani
jtibshirani approved these changes Jan 10, 2019
View reviewed changes
Copy link
Contributor

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

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

Looks good to me, pending @markharwood's last small comments.

@joshdover
Copy link
Contributor

joshdover commented Jan 10, 2019

After this gets merged, would you mind kicking off a new 6.x snapshot?

@cbuescher
Copy link
Member Author

cbuescher commented Jan 10, 2019

@elasticmachine run the gradle build tests 2

@markharwood
Copy link
Contributor

markharwood commented Jan 11, 2019

LGTM

@cbuescher
Copy link
Member Author

cbuescher commented Jan 11, 2019

@jtibshirani @markharwood thanks for the reviews. I'll merge and follow up with a small PR against master enabling mixed-cluster rest tests between 7.0 and 6.x now that the parameter is supported in both versions.

@cbuescher cbuescher merged commit 6797844 into elastic:6.x Jan 11, 2019
@cbuescher cbuescher mentioned this pull request Jan 11, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@markharwood markharwood markharwood left review comments

@jtibshirani jtibshirani jtibshirani approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport :Search Foundations/Mapping Index mappings, including merging and defining field types v6.7.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@cbuescher @elasticmachine @jtibshirani @joshdover @markharwood