Update the default for include_type_name to false. #37285

jtibshirani · 2019-01-10T02:54:58Z

This PR updates the default to include_type_name to false for all APIs that accept it (indices.create, indices.put_mapping, indices.get_mapping, indices.get_field_mapping, indices.put_template, indices.get_template).

It tries to fix up as many tests as possible so that they avoid using the include_type_name parameter altogether, and omit types from the mappings.

The following approach was taken for the REST tests:

If the test is only running against 7.0.0 because it is testing typeless APIs, then omit the include_type_name parameter altogether. This lets us test the default behavior.
If the test is explicitly testing the typed APIs (its name ends with _with_types.yml), then use include_type_name = true where appropriate, to continue to test the typed case.
If the test is not explicitly testing typeless vs. typed APIs, then prefer to explicitly set include_type_name = false and omit types. This works in a mixed-cluster set-up because 6.7 nodes now understand this parameter as well.
The one notable exception to point 3 is for indices.create requests. These are so common and scattered across so many tests that it would be too much for this PR to try to convert all of those tests to stop using types. The way we get around this is by a modification in the test harness: we check if the indices.create call contains the parameter include_type_name, and if not, set it to true when making the API call. Note that this is very much temporary and will need to be fixed in a series of follow-up PRs that update REST tests.

The approach for REST documentation:

As much as possible, avoid using include_type_name altogether and switch to the new API format.
However, when an index is created during set-up, I have simply added include_type_name=true. Like the REST tests, it is a very large amount of manual work to drop include_type_name everywhere, so I thought we would update the documentation in a series of follow-ups. I think it would be possible to take the same approach in the documentation that we do for other REST tests (don't add any parameter, but rely on setting include_type_name=true in the test harness). However, I thought that it was best to avoid non-working documentation examples.

Note that no changes were made to the Java HLRC yet (or HLRC tests), and we do not yet emit deprecation warnings. Those last pieces of work are coming in follow-up PRs.

elasticmachine · 2019-01-10T02:55:00Z

Pinging @elastic/es-search

markharwood · 2019-01-10T10:22:36Z

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

@@ -49,7 +49,7 @@ public String getName() {

    @Override
    public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
-        final boolean includeTypeName = request.paramAsBoolean(INCLUDE_TYPE_NAME_PARAMETER, true);
+        final boolean includeTypeName = request.paramAsBoolean(INCLUDE_TYPE_NAME_PARAMETER, false);


Can we refer to a single constant here that controls our default policy?

markharwood · 2019-01-10T10:24:17Z

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

@@ -86,7 +86,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
            deprecationLogger.deprecated("Type exists requests are deprecated, as types have been deprecated.");
        }

-        final boolean includeTypeName = request.paramAsBoolean(INCLUDE_TYPE_NAME_PARAMETER, true);
+        final boolean includeTypeName = request.paramAsBoolean(INCLUDE_TYPE_NAME_PARAMETER, false);


See earlier comment re single constant

markharwood · 2019-01-10T10:24:42Z

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

@@ -68,11 +68,12 @@ public String getName() {

    @Override
    public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
-        final boolean includeTypeName = request.paramAsBoolean(INCLUDE_TYPE_NAME_PARAMETER, true);
+        final boolean includeTypeName = request.paramAsBoolean(INCLUDE_TYPE_NAME_PARAMETER, false);


See earlier comment re single constant

markharwood · 2019-01-10T13:57:02Z

Further to earlier comments about adding a constant - I merged a PR for 6.x that adds such a constant. Could this PR perhaps add the same constant but with the new "false" setting ie:

public static final boolean DEFAULT_INCLUDE_TYPE_NAME_POLICY = false;

Note that https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/action/admin/indices/get/GetIndexResponse.java#L257 should also need changing to refer to this new constant as well as the RestCreateIndexAction, RestGetMappingAction andRestPutMappingAction classes already in this PR

jtibshirani · 2019-01-10T17:55:43Z

Thanks @markharwood, I'll make sure to introduce the same constant as part of this PR.

cbuescher

@jtibshirani I left a couple of questions, its probably not necessary to look at all the test adaptions in detail so I was mostly trying to see how the plan in the description fits the changes made and left some questions where I wasn't sure about the overall plan.

client/rest-high-level/src/main/java/org/elasticsearch/client/IndicesRequestConverters.java

docs/reference/analysis/charfilters/pattern-replace-charfilter.asciidoc

cbuescher · 2019-01-11T11:00:48Z

docs/reference/indices/create-index.asciidoc

@@ -177,28 +175,3 @@ PUT test?wait_for_active_shards=2

 A detailed explanation of `wait_for_active_shards` and its possible values can be found
 <<index-wait-for-active-shards,here>>.
-
-[float]
-=== Skipping types


Should we have the reverse section as well explaining how to keep types although deprecated in case you still need them?

👍good idea!

...framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestExecutionContext.java

server/src/main/java/org/elasticsearch/cluster/metadata/IndexTemplateMetaData.java

cbuescher · 2019-01-11T11:17:15Z

rest-api-spec/src/main/resources/rest-api-spec/test/suggest/30_context.yml

@@ -195,7 +195,8 @@ setup:
      indices.refresh: {}

  - do:
-      indices.get_mapping: {}
+      indices.get_mapping:
+        include_type_name: false


Is this necessary for mixed cluster?

It is not technically necessary, since we don't use the output of the call. However, I just tried to use include_type_name = false for all mixed-cluster set-ups for clarity/ consistency, as opposed to making individual calls on a test-by-test basis.

markharwood · 2019-01-11T13:35:07Z

docs/reference/indices/create-index.asciidoc

@@ -78,7 +78,7 @@ that can be set when creating an index, please check the
 [[mappings]]
 === Mappings

-The create index API allows to provide a type mapping:
+The create index API allows to provide a mapping:


The JSON property is still called "mappings", plural so I'd be tempted to describe them as "field mappings".

markharwood · 2019-01-11T14:31:57Z

Digging into the build failure I see GetFieldMappingResponseTests is failing because
GetFieldMappingResponse expects only one type as it loops around the mapping response.
However, GetFieldMappingsResponseTests generates a random mapping with multiple types:

markharwood · 2019-01-11T17:37:32Z

Other build failure here is GetIndexTemplateResponseTests - the toXContent/fromXContent methods disagree - toXContent now provides typeless mappings due to default include_type_name=false param but IndexTemplateMetaData.fromXContent is hard-coded to assume a type is there so picks up properties as a type name.

jtibshirani · 2019-01-11T17:57:45Z

Thanks @cbuescher and @markharwood for the review and for digging into the build failures. For the Get*Response unit tests, I plan to make sure that they only consider typed responses, as they've done before this PR. Then, when we open PRs to add Java HLRC support, we will introduce fromXContent methods that understand typeless mappings, and can migrate these tests to touch on both typeless and typed responses.

…-default

cbuescher

Thanks for the updates, looks good from my side.

jtibshirani · 2019-01-14T20:56:15Z

@elastic/es-clients I forgot to ping the clients team, my apologies! I think it's important to be aware of this PR because of the modification in the test harness:

The one notable exception to point 3 is for indices.create requests. These are so common and scattered across so many tests that it would be too much for this PR to try to convert all of those tests to stop using types. The way we get around this is by a modification in the test harness: we check if the indices.create call contains the parameter include_type_name, and if not, set it to true when making the API call. Note that this is very much temporary and will need to be fixed in a series of follow-up PRs that update REST tests.

Feel free to ping me if you have any questions about the REST tests changes.

* master: (28 commits) Introduce retention lease serialization (elastic#37447) Update Delete Watch to allow unknown fields (elastic#37435) Make finalize step of recovery source non-blocking (elastic#37388) Update the default for include_type_name to false. (elastic#37285) Security: remove SSL settings fallback (elastic#36846) Adding mapping for hostname field (elastic#37288) Relax assertSameDocIdsOnShards assertion Reduce recovery time with compress or secure transport (elastic#36981) Implement ccr file restore (elastic#37130) Fix Eclipse specific compilation issue (elastic#37419) Performance fix. Reduce deprecation calls for the same bulk request (elastic#37415) [ML] Use String rep of Version in map for serialisation (elastic#37416) Cleanup Deadcode in Rest Tests (elastic#37418) Mute IndexShardRetentionLeaseTests.testCommit elastic#37420 unmuted test Remove unused index store in directory service Improve CloseWhileRelocatingShardsIT (elastic#37348) Fix ClusterBlock serialization and Close Index API logic after backport to 6.x (elastic#37360) Update the scroll example in the docs (elastic#37394) Update analysis.asciidoc (elastic#37404) ...

Follow-up to elastic#28497 Since elastic/elasticsearch#37285 has been merged, it's aparent there are a couple more places we need to specify include_type_name as a stop-gap until elastic#23650 is completed Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

* [migrations] Fetch additional mappings with types Follow-up to #28497 Since elastic/elasticsearch#37285 has been merged, it's aparent there are a couple more places we need to specify include_type_name as a stop-gap until #23650 is completed Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co> * Updates tests Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

karmi · 2019-01-15T17:22:52Z

@jtibshirani, thanks for the ping! I've tried to do what you suggest — adding include_type_name=false to index.create calls which don't set it —, and it makes my tests green again.

The "include_type_name" parameter was temporarily introduced in elastic#37285 to facilitate moving the default parameter setting to "false" in many places in the documentation code snippets. Most of the places can simply be reverted without causing errors. In this change I looked for asciidoc files that contained the "include_type_name=true" addition when creating new indices but didn't look likey they made use of the "_doc" type for mappings. This is mostly the case e.g. in the analysis docs where index creating often only contains settings. I manually corrected the use of types in some places where the docs still used an explicit type name and not the dummy "_doc" type.

The "include_type_name" parameter was temporarily introduced in #37285 to facilitate moving the default parameter setting to "false" in many places in the documentation code snippets. Most of the places can simply be reverted without causing errors. In this change I looked for asciidoc files that contained the "include_type_name=true" addition when creating new indices but didn't look likey they made use of the "_doc" type for mappings. This is mostly the case e.g. in the analysis docs where index creating often only contains settings. I manually corrected the use of types in some places where the docs still used an explicit type name and not the dummy "_doc" type.

From #29453 and #37285, the `include_type_name` parameter was already present and defaulted to false. This PR makes the following updates: - Add deprecation warnings to `RestPutMappingAction`, plus tests in `RestPutMappingActionTests`. - Add a typeless 'put mappings' method to the Java HLRC, and deprecate the old typed version. To do this cleanly, I opted to create a new `PutMappingRequest` object that differs from the existing server one.

From #29453 and #37285, the include_type_name parameter was already present and defaulted to false. This PR makes the following updates: * Add deprecation warnings to RestCreateIndexAction, plus tests in RestCreateIndexActionTests. * Add a typeless 'create index' method to the Java HLRC, and deprecate the old typed version. To do this cleanly, I created new CreateIndexRequest and CreateIndexResponse objects that differ from the existing server ones.

This PR removes the temporary change we made to the yml test harness in #37285 to automatically set `include_type_name` to `true` in index creation requests if it's not already specified. This is possible now that the vast majority of index creation requests were updated to be typeless in #37611. A few additional tests also needed updating here. Additionally, this PR updates the test harness to set `include_type_name` to `false` in index creation requests when communicating with 6.x nodes. This mirrors the logic added in #37611 to allow for typeless document write requests in test set-up code. With this update in place, we can remove many references to `include_type_name: false` from the yml tests.

jtibshirani added >enhancement >breaking WIP :Search Foundations/Mapping Index mappings, including merging and defining field types v7.0.0 labels Jan 10, 2019

jtibshirani mentioned this pull request Jan 10, 2019

Deprecate types in the put mapping API. #37280

Merged

jtibshirani force-pushed the include-type-name-default branch from 547d160 to 188cb0a Compare January 10, 2019 08:03

markharwood reviewed Jan 10, 2019

View reviewed changes

markharwood mentioned this pull request Jan 10, 2019

Types removal - PUT and GET IndexTemplates. #35866

Closed

jtibshirani force-pushed the include-type-name-default branch from 188cb0a to 6cd858e Compare January 11, 2019 07:56

jtibshirani changed the title ~~Default include_type_name to false for create index, and put and get mappings.~~ Update the default for include_type_name to false. Jan 11, 2019

jtibshirani removed the WIP label Jan 11, 2019

jtibshirani force-pushed the include-type-name-default branch from 6cd858e to e20aebb Compare January 11, 2019 08:08

jtibshirani requested a review from cbuescher January 11, 2019 08:08

jtibshirani force-pushed the include-type-name-default branch from e20aebb to 64093c9 Compare January 11, 2019 08:11

jtibshirani requested a review from jpountz January 11, 2019 08:12

cbuescher reviewed Jan 11, 2019

View reviewed changes

chrisdavies mentioned this pull request Jan 11, 2019

Remove reference to mapping types elastic/kibana#23650

Closed

markharwood reviewed Jan 11, 2019

View reviewed changes

jtibshirani mentioned this pull request Jan 11, 2019

Implementation tracking for 7.0 types deprecation. #35190

Closed

48 tasks

jtibshirani force-pushed the include-type-name-default branch 2 times, most recently from 727f626 to 2e54beb Compare January 12, 2019 20:59

Merge remote-tracking branch 'upstream/master' into include-type-name…

e3281dc

…-default

cbuescher approved these changes Jan 14, 2019

View reviewed changes

jtibshirani merged commit 36a3b84 into elastic:master Jan 14, 2019

jtibshirani deleted the include-type-name-default branch January 14, 2019 21:08

tylersmalley mentioned this pull request Jan 15, 2019

[migrations] Fetch additional mappings with types elastic/kibana#28729

Merged

jtibshirani mentioned this pull request Jan 16, 2019

Allow indices.get_mapping response parsing without types #37492

Merged

cbuescher mentioned this pull request Jan 16, 2019

Remove "include_type_name" from most places in docs #37556

Closed

cbuescher mentioned this pull request Jan 17, 2019

Remove include_type_name in asciidoc where possible #37568

Merged

jtibshirani mentioned this pull request Jan 18, 2019

Deprecate types in create index requests. #37134

Merged

cbuescher mentioned this pull request Jan 21, 2019

Remove remaining occurances of "include_type_name=true" in docs #37646

Merged

jtibshirani mentioned this pull request Jan 30, 2019

Default include_type_name to false in the yml test harness. #38058

Merged

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

jakelandis mentioned this pull request Apr 15, 2019

fix FullClusterRestart tests now that 7.0.0 can be an "oldCluster" #41224

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the default for include_type_name to false. #37285

Update the default for include_type_name to false. #37285

jtibshirani commented Jan 10, 2019 •

edited

Loading

elasticmachine commented Jan 10, 2019

markharwood Jan 10, 2019 •

edited

Loading

jtibshirani Jan 11, 2019

markharwood Jan 10, 2019

markharwood Jan 10, 2019

markharwood commented Jan 10, 2019 •

edited

Loading

jtibshirani commented Jan 10, 2019

cbuescher left a comment •

edited

Loading

cbuescher Jan 11, 2019

jtibshirani Jan 11, 2019 •

edited

Loading

cbuescher Jan 11, 2019

jtibshirani Jan 11, 2019

markharwood Jan 11, 2019

jtibshirani Jan 11, 2019

markharwood commented Jan 11, 2019 •

edited

Loading

markharwood commented Jan 11, 2019

jtibshirani commented Jan 11, 2019

cbuescher left a comment

jtibshirani commented Jan 14, 2019

karmi commented Jan 15, 2019

Update the default for include_type_name to false. #37285

Update the default for include_type_name to false. #37285

Conversation

jtibshirani commented Jan 10, 2019 • edited Loading

elasticmachine commented Jan 10, 2019

markharwood Jan 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markharwood commented Jan 10, 2019 • edited Loading

jtibshirani commented Jan 10, 2019

cbuescher left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markharwood commented Jan 11, 2019 • edited Loading

markharwood commented Jan 11, 2019

jtibshirani commented Jan 11, 2019

cbuescher left a comment

Choose a reason for hiding this comment

jtibshirani commented Jan 14, 2019

karmi commented Jan 15, 2019

jtibshirani commented Jan 10, 2019 •

edited

Loading

markharwood Jan 10, 2019 •

edited

Loading

markharwood commented Jan 10, 2019 •

edited

Loading

cbuescher left a comment •

edited

Loading

jtibshirani Jan 11, 2019 •

edited

Loading

markharwood commented Jan 11, 2019 •

edited

Loading