Support 'include_type_name' in RestGetIndicesAction #37149

cbuescher · 2019-01-04T18:14:02Z

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.

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.

elasticmachine · 2019-01-04T18:14:03Z

Pinging @elastic/es-search

cbuescher · 2019-01-04T18:34:58Z

rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/10_basic.yml

@@ -52,6 +52,35 @@ setup:
  - is_true: test_index.settings
  - is_true: test_index.mappings

+---
+"Test include_type_name":
+  - skip:


Note: this can be removed after the parameter support has been backported to 6.x

I started referring to this parameter name from various places in elastic#37149 so I think it's a good idea to simplify things by referring to a common constant.

cbuescher · 2019-01-04T22:33:54Z

@elasticmachine run the gradle build tests 1

I started referring to this parameter name from various places in #37149 so I think it's a good idea to simplify things by referring to a common constant.

cbuescher · 2019-01-07T11:55:00Z

@elasticmachine run the gradle build tests 1

jtibshirani

Thanks @cbuescher, this looks like it's on a good track. My main questions are around the Java HLRC:

I had assumed we would flatten GetIndexResponse#mappings to be of type ImmutableOpenMap<String, MappingMetaData>, to avoid nesting the mappings under the _doc key.
When we backport to 6.x, are we planning to add note under 'breaking changes', and also a 'breaking' label to the PR? My understanding from reading this PR is that for the Java HLRC we will force include_type_name = false.

Finally, maybe a Java HLRC expert could take a look to make sure this breaking approach is okay.

server/src/main/java/org/elasticsearch/action/admin/indices/get/GetIndexResponse.java

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetIndicesAction.java

rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/10_basic.yml

server/src/main/java/org/elasticsearch/action/admin/indices/get/GetIndexResponse.java

cbuescher · 2019-01-07T21:36:53Z

* I had assumed we would flatten `GetIndexResponse#mappings` to be of type `ImmutableOpenMap<String, MappingMetaData>`, to avoid nesting the mappings under the `_doc` key.

If we want to do this, I would only do it in the getter in GetIndexResponse, but leave the internal mappings field as is. Otherwise we also need to touch serialization and possible how the mappings are filled in TransportGetIndexAction. I think these are internal refactorings that we should do later, maybe even only on 8.0 where we remove all this cruft.

Changing the Java-API of GetIndexResponse however would be breaking, at least in 7.0. I wouldn't backport this change to 6.x. It would also mean ppl. would be able to get at the "old" type names via the REST API but not e.g. using the old transport client, which might be considered odd.

cbuescher · 2019-01-07T21:42:29Z

My understanding from reading this PR is that for the Java HLRC we will force `include_type_name = false`.

So far I thought we could make the 6.x HLRC client force include_type_name = true to get back the legacy format. The 6.x client needs to be forward compatible, so they should be able to talk to both 6.x and 7.0 nodes. I think it would help people transitioning if they would not need to update any of their own application logic before moving to the 7.0 client which will force the new response behaviour and will swallow the type information.

cbuescher · 2019-01-07T22:12:59Z

@jtibshirani thanks for the review, I pushed a round of changes and tried to respond to your two questions above. Let me know if these answers make sense or you have objections. If we cannot decide on the GetIndexResponse#mappings change for now, we can leave it out of this PR perhaps and discuss it in more details elsewhere? It should be easy to add if we agree on doing so later on as well.

rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/10_basic.yml

jtibshirani

A few more comments, it looks very close! Sounds good about revisiting the mappings HLRC change later. It would be good to have a high-bandwidth discussion about this in person.

...c/test/java/org/elasticsearch/action/admin/indices/mapping/get/GetMappingsResponseTests.java

server/src/test/java/org/elasticsearch/action/admin/indices/get/GetIndexResponseTests.java

rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/10_basic.yml

server/src/main/java/org/elasticsearch/action/admin/indices/get/GetIndexResponse.java

jtibshirani

I left a few comments for your consideration, but they are minor and I don't think need another round of review.

cbuescher · 2019-01-08T20:45:46Z

@jtibshirani thanks for the last round of review and the thumbs up, I pushed another commit adressing you last comments. Waiting for green CI now before I merge.

server/src/test/java/org/elasticsearch/rest/action/admin/indices/RestGetIndicesActionTests.java

cbuescher · 2019-01-09T12:07:04Z

@elasticmachine run default distro tests

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.

cbuescher added :Search Foundations/Mapping Index mappings, including merging and defining field types >deprecation v7.0.0 labels Jan 4, 2019

jtibshirani mentioned this pull request Jan 4, 2019

Implementation tracking for 7.0 types deprecation. #35190

Closed

48 tasks

cbuescher commented Jan 4, 2019

View reviewed changes

cbuescher mentioned this pull request Jan 4, 2019

Introduce an include_type_name constant #37155

Merged

cbuescher force-pushed the types-deprecation-get-indices branch 2 times, most recently from 8f3cb9b to 7ebb0cb Compare January 4, 2019 20:37

iter

4a07eed

cbuescher force-pushed the types-deprecation-get-indices branch from 7ebb0cb to 4a07eed Compare January 4, 2019 20:44

cbuescher pushed a commit that referenced this pull request Jan 7, 2019

Introduce an include_type_name constant (#37155)

d18c3d6

I started referring to this parameter name from various places in #37149 so I think it's a good idea to simplify things by referring to a common constant.

Christoph Büscher added 2 commits January 7, 2019 11:13

Merge branch 'master' into types-deprecation-get-indices

67670a8

Use constant

e9f9f64

jtibshirani reviewed Jan 7, 2019

View reviewed changes

Christoph Büscher added 2 commits January 7, 2019 22:14

iter

e3988b7

Remove parsing with types

d169030

change mapping toXContent

296f2bc

cbuescher force-pushed the types-deprecation-get-indices branch from 92c6e86 to 296f2bc Compare January 7, 2019 22:09

Fix failing yaml test

06cc067

cbuescher commented Jan 7, 2019

View reviewed changes

rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/10_basic.yml Outdated Show resolved Hide resolved

jtibshirani reviewed Jan 7, 2019

View reviewed changes

Revert changes in yaml tests

9cb40f2

jtibshirani reviewed Jan 8, 2019

View reviewed changes

server/src/main/java/org/elasticsearch/action/admin/indices/get/GetIndexResponse.java Outdated Show resolved Hide resolved

jtibshirani approved these changes Jan 8, 2019

View reviewed changes

iter

52ffb4b

jtibshirani reviewed Jan 9, 2019

View reviewed changes

server/src/test/java/org/elasticsearch/rest/action/admin/indices/RestGetIndicesActionTests.java Show resolved Hide resolved

jtibshirani added the >breaking label Jan 9, 2019

Add Skipping types section to get-indices docs

585ed20

cbuescher merged commit c149bb8 into elastic:master Jan 9, 2019

cbuescher mentioned this pull request Jan 9, 2019

Support 'include_type_name' in RestGetIndicesAction #37267

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support 'include_type_name' in RestGetIndicesAction #37149

Support 'include_type_name' in RestGetIndicesAction #37149

cbuescher commented Jan 4, 2019

elasticmachine commented Jan 4, 2019

cbuescher Jan 4, 2019

cbuescher commented Jan 4, 2019

cbuescher commented Jan 7, 2019

jtibshirani left a comment

cbuescher commented Jan 7, 2019

cbuescher commented Jan 7, 2019

cbuescher commented Jan 7, 2019

jtibshirani left a comment •

edited

Loading

jtibshirani left a comment •

edited

Loading

cbuescher commented Jan 8, 2019

cbuescher commented Jan 9, 2019

Support 'include_type_name' in RestGetIndicesAction #37149

Support 'include_type_name' in RestGetIndicesAction #37149

Conversation

cbuescher commented Jan 4, 2019

elasticmachine commented Jan 4, 2019

cbuescher Jan 4, 2019

Choose a reason for hiding this comment

cbuescher commented Jan 4, 2019

cbuescher commented Jan 7, 2019

jtibshirani left a comment

Choose a reason for hiding this comment

cbuescher commented Jan 7, 2019

cbuescher commented Jan 7, 2019

cbuescher commented Jan 7, 2019

jtibshirani left a comment • edited Loading

Choose a reason for hiding this comment

jtibshirani left a comment • edited Loading

Choose a reason for hiding this comment

cbuescher commented Jan 8, 2019

cbuescher commented Jan 9, 2019

jtibshirani left a comment •

edited

Loading

jtibshirani left a comment •

edited

Loading