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

Implementation tracking for 7.0 types deprecation. #35190

Closed
48 tasks done
jtibshirani opened this issue Nov 2, 2018 · 12 comments
Closed
48 tasks done

Implementation tracking for 7.0 types deprecation. #35190

jtibshirani opened this issue Nov 2, 2018 · 12 comments
Assignees
Labels
>deprecation Meta :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch

Comments

@jtibshirani
Copy link
Contributor

jtibshirani commented Nov 2, 2018

Tracks the details of the 'In 7.0' part of this comment: #15613 (comment)

Plan for 7.0

  • For requests with a type in the URL or as a leaf field, we will accept both typed + typeless versions of the API. We’ll emit a deprecation warning to tell users they need to move to the typeless endpoints before 8.0. Responses will still contain a _type field, but we will return the dummy name _doc regardless of the underlying type name.
  • For APIs whose request/ response structure changes with the deprecation (create index, get mapping, etc.), we’ll have a request parameter include_type_name that should be set to false to omit types in requests + responses. It will default to true in 6.7 with a warning that it needs to be explicitly specified (to either true or false), default to false in 7.0 with a warning to stop specifying it, and finally be removed in 8.0.

More information can be found here: https://www.elastic.co/guide/en/elasticsearch/reference/current/removal-of-types.html

To-do List

Phase One: Add Typeless APIs. These items are critical for 6.7/ 7.0, and should be in before feature freeze.

Phase Two: Important Clean-up. These tasks should be in by 6.7/ 7.0, but can go in after feature freeze.

Phase Three: Additional Deprecations. These are good to have by 6.7/ 7.0, but could be pushed into 7.1 if strictly necessary.

Items we’re still following-up on

  • For responses that contain the type as a leaf field, should we always return _doc regardless of the underlying type even when the old typed APIs are used?
  • Types may be present in saved search requests, including search templates and watches. We should think through the upgrade plan here. Implementation tracking for 7.0 types deprecation. #35190 (comment)
  • When an index template is stored, the mappings are nested under the type name. We also need to consider how these will be accessed and upgraded.
@jtibshirani jtibshirani added :Search Foundations/Mapping Index mappings, including merging and defining field types Meta >deprecation labels Nov 2, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@jtibshirani
Copy link
Contributor Author

CC @elastic/es-clients

@jtibshirani jtibshirani self-assigned this Nov 8, 2018
@Mpdreamz
Copy link
Member

Mpdreamz commented Nov 12, 2018

One last thing I want to raise which might be super contentious:

Right now the default for include_type_name when not specified is true in 7.0. There are two downsides with this:

  • New installations will get deprecation warnings OOTB unless they specify include_type_name=false which they need to remove in 8.0.
  • Someone who wants to go through all the type related changes in one major version can not do so.

Can we remove some of the confusion of include_type_name defaulting to true by simply having it
default to false in 7.0 already?

  • New installations will return the desired responses by default.
  • Someone who wants to go through all the changes in one major version can do so.

Upgrades will have to account for the type removal whether the include_type_name defaults to true or false.

Are we actually doing our users a disservice by holding on to a deprecation period?

@markharwood
Copy link
Contributor

Types may be present in saved search requests, including search templates and watches. We should think through the upgrade plan here.

The proposal is in 7.0 we will introduce deprecation warnings when saved queries with types are executed, but not do anything to proactively detect and upgrade them automatically. Some of the easier type removals would be in the structured metadata held around a saved search whereas other changes may be buried in clauses embedded in the saved search's choice of query DSL.

@jpountz
Copy link
Contributor

jpountz commented Nov 15, 2018

@Mpdreamz I'm thinking that this could be an issue for mixed version clusters that have 6.latest and 7.0 nodes as the response format would depend on the version of the node that you are querying?

@bleskes
Copy link
Contributor

bleskes commented Nov 19, 2018

New installations will get deprecation warnings OOTB unless
@Mpdreamz I'm thinking that this could be an issue for mixed version clusters that have 6.latest and 7.0 nodes as the response format would depend on the version of the node that you are querying?

This is typically dealt with in the following fashion:

  1. Add a flag to enable new functionality in the old version, while defaulting to disable. Requests without that flag will emit deprecation warnings.
  2. New version only supports the flag in "enabled mode" and will omit deprecation logs when flag is used.

The suggestion above follows this pattern, which is good. The only difference with what we do normally is the duration of deprecation. Normally it will be scoped down to something like OLD_MAJOR.latest. With the above suggestion it's NEW_MAJOR.x , which is a long time. I wonder if we should look at the costs of backporting the changes made to 6.x so we can follow our standard path.

@jpountz
Copy link
Contributor

jpountz commented Nov 20, 2018

In the beginning this flag was supposed to exist on most APIs including some of the most used like the document APIs. This made me view the long deprecation period as a feature. Now that it's only about the mappings APIs, I could change my mind. One subtlety with backporting to 6.x is that it might still have 5.x indices that have multiple types, so include_type_name=false would not make sense on such indices and we'd need to respond appropriately (error?).

jpountz added a commit to jpountz/elasticsearch that referenced this issue Nov 21, 2018
…rom `_doc`.

This commit makes `document`, `update`, `explain`, `termvectors` and `mapping`
typeless APIs work on indices that have a type whose name is not `_doc`.
Unfortunately, this needs to be a bit of a hack since I didn't want calls with
random type names to see documents with the type name that the user had chosen
upon type creation.

The `explain` and `termvectors` do not support being called without a type for
now so the test is just using `_doc` as a type for now, we will need to fix
tests later but this shouldn't require further changes server-side since passing
`_doc` as a type name is what typeless APIs do internally anyway.

Relates elastic#35190
@markharwood
Copy link
Contributor

I looked into auto-detecting PUTs of typeless mappings/templates to see if we can avoid the need for passing an include_type_name flag in the URL. It looks like this will be possible as we can check if the root object has a value called "properties". Originally I thought that real estate agents with a doc type called "properties" might be an edge case which would cause ambiguity. Fortunately 6.5 has a parsing bug that prevents you from creating doc types called "properties" (try it and see).

@markharwood
Copy link
Contributor

Fortunately 6.5 has a parsing bug that prevents you from creating doc types called "properties"

Turns out there's more to auto-detecting no-types than checking for the presence of a top-level properties field. Legal mappings can have no properties but include other top-level attributes e.g. _source:{enabled:false} or dynamic_templates:{...}.

While it would have been nice to auto-detect typeless mappings and provide the 7.0 examples in reference docs without any types this would not be consistent with the results of GET _mapping calls which would return types by default (unless the include_type_name=false param is passed).

Does auto-detection still seem useful or shall we insist on documenting APIs with include_type_name=false params?

jpountz added a commit that referenced this issue Dec 4, 2018
…rom `_doc` (#35790)

This commit makes `document`, `update`, `explain`, `termvectors` and `mapping`
typeless APIs work on indices that have a type whose name is not `_doc`.
Unfortunately, this needs to be a bit of a hack since I didn't want calls with
random type names to see documents with the type name that the user had chosen
upon type creation.

The `explain` and `termvectors` do not support being called without a type for
now so the test is just using `_doc` as a type for now, we will need to fix
tests later but this shouldn't require further changes server-side since passing
`_doc` as a type name is what typeless APIs do internally anyway.

Relates #35190
@cbuescher cbuescher self-assigned this Dec 6, 2018
mayya-sharipova added a commit to mayya-sharipova/elasticsearch that referenced this issue Dec 7, 2018
mayya-sharipova added a commit to mayya-sharipova/elasticsearch that referenced this issue Dec 11, 2018
markharwood added a commit to markharwood/elasticsearch that referenced this issue Feb 4, 2019
…stic#37484)

Added deprecation warnings for use of include_type_name in put/get index templates.
HLRC changes:
GetIndexTemplateRequest has a new client-side class which is a copy of server's GetIndexTemplateResponse but modified to be typeless.
PutIndexTemplateRequest has a new client-side counterpart which doesn't use types in the mappings
Relates to elastic#35190
jtibshirani pushed a commit that referenced this issue Feb 5, 2019
Added deprecation warnings for use of include_type_name in put/get index templates.
HLRC changes:
* GetIndexTemplateRequest has a new client-side class which is a copy of server's GetIndexTemplateResponse but modified to be typeless.
* PutIndexTemplateRequest has a new client-side counterpart which doesn't use types in the mappings

Relates to #35190
mayya-sharipova added a commit to mayya-sharipova/elasticsearch that referenced this issue Feb 5, 2019
mayya-sharipova added a commit that referenced this issue Feb 5, 2019
mayya-sharipova added a commit to mayya-sharipova/elasticsearch that referenced this issue Feb 5, 2019
cbuescher pushed a commit that referenced this issue Feb 5, 2019
)

The HLRC client currently uses `org.elasticsearch.action.admin.indices.get.GetIndexRequest`
and `org.elasticsearch.action.admin.indices.get.GetIndexResponse` in its get index
calls. Both request and response are designed for the typed APIs, including some
return types e.g. for `getMappings()` which in the maps it returns still use a
level including the type name.  In order to change this without breaking
existing users of the HLRC API, this PR introduces two new request and response
objects in the `org.elasticsearch.client.indices` client package. These are used
by the IndicesClient#get and IndicesClient#exists calls now by default and
support the type-less API. The old request and response objects are still kept
for use in similarly named, but deprecated methods.

The newly introduced client side classes are simplified versions of the server
side request/response classes since they don't need to support wire
serialization, and only the response needs fromXContent parsing (but no
xContent-serialization, since this is the responsibility of the server-side
class).  Also changing the return type of `GetIndexResponse#getMapping` to
`Map<String, MappingMetaData> getMappings()`, while it previously was returning
another map keyed by the type-name. Similar getters return simple Maps instead
of the ImmutableOpenMaps that the server side response objects return.

Backport for #37778
Relates to #35190
mayya-sharipova added a commit that referenced this issue Feb 5, 2019
markharwood added a commit to markharwood/elasticsearch that referenced this issue Feb 5, 2019
…stic#37484)

Added deprecation warnings for use of include_type_name in put/get index templates.
HLRC changes:
GetIndexTemplateRequest has a new client-side class which is a copy of server's GetIndexTemplateResponse but modified to be typeless.
PutIndexTemplateRequest has a new client-side counterpart which doesn't use types in the mappings
Relates to elastic#35190
jtibshirani pushed a commit that referenced this issue Feb 6, 2019
Added deprecation warnings for use of include_type_name in put/get index templates.
HLRC changes:
* GetIndexTemplateRequest has a new client-side class which is a copy of server's GetIndexTemplateResponse but modified to be typeless.
* PutIndexTemplateRequest has a new client-side counterpart which doesn't use types in the mappings

Relates to #35190
@jakelandis
Copy link
Contributor

When an index template is stored, the mappings are nested under the type name. We also need to consider how these will be accessed and upgraded.

I logged #38637 to further address this point.

jtibshirani added a commit that referenced this issue Feb 13, 2019
…8825)

We expect many users to have a custom document type in 6.x, for example they
might have used `doc` when creating indices. In this case, users cannot easily
switch over to typeless index creations in 6.7 using `include_type_name=false`,
because the rest of their document CRUD calls will refer to the custom type.
Instead, we are recommending that users take the following steps: set
`include_type_name=true` in 6.7 for all relevant calls, upgrade to 7.0, then
switch over completely to the typeless APIs and stop using `include_type_name`.

This means that it will be very common to set `include_type_name=true` in 6.7,
so it is misleading to emit a deprecation warning to tell the user to switch to
using `include_type_name=false`. This PR switches to emitting a deprecation
warning only if `include_type_name` is not set at all. The warning serves as an
important note to users that the request and response format of these APIs will
change in a breaking way in 7.0.

Relates to #35190.
@jpountz
Copy link
Contributor

jpountz commented Apr 2, 2019

All items have been completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>deprecation Meta :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch
Projects
None yet
Development

No branches or pull requests

10 participants