Deprecate use of type in reindex request body #36823

cbuescher · 2018-12-19T10:17:04Z

Types can be used both in the source and dest section of the body which will
be translated to search and index requests respectively. Adding a deprecation warning
for those cases and removing examples using more than one type in reindex since
support for this is going to be removed.

elasticmachine · 2018-12-19T10:17:07Z

Pinging @elastic/es-search

mayya-sharipova

Thanks @cbuescher, looks good overall.
The main question I have is about the interpretation of "Deprecate use of type". I imagined that for any reference to type, even though it is equal to _doc, we should issue a deprecation warning. @jtibshirani what do you think?

docs/reference/docs/reindex.asciidoc

cbuescher · 2018-12-19T20:56:20Z

I imagined that for any reference to type, even though it is equal to _doc, we should issue a deprecation warning.

I was going after something like I saw in RestGetAction where we check !type.equals(MapperService.SINGLE_MAPPING_NAME) before we log the deprecation. But maybe thats different there because "_doc" can still appear in the path. Here we can probably deprecate all uses because "type" is used in the request body.

jtibshirani · 2018-12-19T21:02:42Z

I agree that we want to log a deprecation warning anytime that type appears in the request body. Even if type is set to _doc, we'll want to issue a warning because that key shouldn't be used at all.

I also wanted to note that you may be seeing search-related deprecation warnings when using type in the source section of a reindex request. This is because reindex can create a query on the _type field as part of its execution. I just merged #36802 to avoid issuing this search-related warning, so it might be good to pull in that commit.

cbuescher · 2018-12-19T21:20:31Z

@jtibshirani @mayya-sharipova thanks for the feedback, I pushed an update.

jtibshirani

Thanks @cbuescher, this is looking good to me! I think we'll also need to update ReindexIT to remove references to types.

modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestReindexAction.java

modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestReindexActionTests.java

mayya-sharipova

Thanks @cbuescher, LGTM

cbuescher · 2019-01-02T12:47:33Z

@jtibshirani I think I adressed you latest round of comments, running the test now but I think this should be good to go now?

Types can be used both in the source and dest section of the body which will be translated to search and index requests respectively. Adding a deprecation warning for those cases and removing examples using more than one type in reindex since support for this is going to be removed.

cbuescher · 2019-01-02T17:51:29Z

@elasticmachine test this please

cbuescher · 2019-01-02T17:59:14Z

@elasticmachine test this please

Conky5 · 2019-01-02T18:40:02Z

@elasticmachine test this please

cbuescher · 2019-01-02T22:16:59Z

Even if type is set to _doc, we'll want to issue a warning because that key shouldn't be used at all.

@jtibshirani I just that now the IndexRequest (used for the reindex destination) always contains a non-null type ("_doc" by default, this might be a recent change, I haven't seen related tests failing before). Also the SearchRequest used in the source section always prints the "type" field to xContent even if no types are set, so we will alway see a warning when going e.g. through the High Level Rest Client. I think we need to change the ReindexRequest xContent output slightly so that the "type" fields are not printed when they are not set (e.g. when coming from HLRest)

cbuescher · 2019-01-02T22:32:02Z

@jtibshirani thanks for the thumbs up but the last failures in the high level client tests uncovered what I think were some problems with the xContent output of the current ReindexRequest. Can you take a look at the recent commit and let me know if you agree with not outputting empty or default type fields in the ReindexRequest is serialized via toXContent? This would mean we still warn when empty/default types names are sent directly via REST, but we wouldn't send them outselves when using e.g. the HL client.

jtibshirani · 2019-01-02T22:43:02Z

server/src/main/java/org/elasticsearch/index/reindex/ReindexRequest.java

@@ -294,15 +295,19 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
                builder.field("remote", remoteInfo);
            }
            builder.array("index", getSearchRequest().indices());
-            builder.array("type", getSearchRequest().types());
+            String[] types = getSearchRequest().types();
+            if (types != null && types.length > 0) {


Super small comment -- I don't think these null checks are necessary, since SearchRequest validates that the types array can't be null. Similarly, IndexRequest validates that the type is not null.

IndexRequest validates that the type is not null

Unfortunately this is done in validate() only, so its possible to set a null type via the ctors or the setter and then xContent serialization would break. For SearchRequest I think you are right and types cannot be null so I will remove the check there but leave it for IndexRequest.

jtibshirani · 2019-01-02T22:44:04Z

@cbuescher thanks for double-checking, I agree with this approach to ReindexRequest#toXContent.

kcm · 2019-08-09T03:56:08Z

This bit us on site today because the 7.x docs don't mention type, but it's still needed for reindexing from older multi-type indices into 7.x+. Even if the support for multiple types in dest is gone, won't we still want to keep the support for type in source? What's a workaround if not, for upgrading from pre-6 indices?

Can we at least re-document it for source for now?

cbuescher added :Search/Search Search-related issues that do not fall into other categories v7.0.0 labels Dec 19, 2018

cbuescher added the >deprecation label Dec 19, 2018

mayya-sharipova reviewed Dec 19, 2018

View reviewed changes

docs/reference/docs/reindex.asciidoc Outdated Show resolved Hide resolved

cbuescher force-pushed the deprecate-types-reindex branch from 09318d3 to bc82cfa Compare December 19, 2018 21:40

jtibshirani reviewed Dec 19, 2018

View reviewed changes

mayya-sharipova approved these changes Dec 19, 2018

View reviewed changes

jtibshirani mentioned this pull request Dec 20, 2018

Implementation tracking for 7.0 types deprecation. #35190

Closed

48 tasks

cbuescher force-pushed the deprecate-types-reindex branch from 1304efb to 7dccd45 Compare January 2, 2019 17:08

jtibshirani approved these changes Jan 2, 2019

View reviewed changes

Don't output empty of default type field in toXContent

4be0149

jtibshirani reviewed Jan 2, 2019

View reviewed changes

iter

c962f6a

cbuescher merged commit 046f86f into elastic:master Jan 3, 2019

jtibshirani mentioned this pull request Jan 4, 2019

Types removal - deprecate types in _bulk apis #36549

Merged

jimczi 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

Deprecate use of type in reindex request body #36823

Deprecate use of type in reindex request body #36823

cbuescher commented Dec 19, 2018

elasticmachine commented Dec 19, 2018

mayya-sharipova left a comment

cbuescher commented Dec 19, 2018

jtibshirani commented Dec 19, 2018 •

edited

Loading

cbuescher commented Dec 19, 2018

jtibshirani left a comment

mayya-sharipova left a comment

cbuescher commented Jan 2, 2019

cbuescher commented Jan 2, 2019

cbuescher commented Jan 2, 2019

Conky5 commented Jan 2, 2019

cbuescher commented Jan 2, 2019

cbuescher commented Jan 2, 2019

jtibshirani Jan 2, 2019

cbuescher Jan 2, 2019

jtibshirani commented Jan 2, 2019

kcm commented Aug 9, 2019

Deprecate use of type in reindex request body #36823

Deprecate use of type in reindex request body #36823

Conversation

cbuescher commented Dec 19, 2018

elasticmachine commented Dec 19, 2018

mayya-sharipova left a comment

Choose a reason for hiding this comment

cbuescher commented Dec 19, 2018

jtibshirani commented Dec 19, 2018 • edited Loading

cbuescher commented Dec 19, 2018

jtibshirani left a comment

Choose a reason for hiding this comment

mayya-sharipova left a comment

Choose a reason for hiding this comment

cbuescher commented Jan 2, 2019

cbuescher commented Jan 2, 2019

cbuescher commented Jan 2, 2019

Conky5 commented Jan 2, 2019

cbuescher commented Jan 2, 2019

cbuescher commented Jan 2, 2019

jtibshirani Jan 2, 2019

Choose a reason for hiding this comment

cbuescher Jan 2, 2019

Choose a reason for hiding this comment

jtibshirani commented Jan 2, 2019

kcm commented Aug 9, 2019

jtibshirani commented Dec 19, 2018 •

edited

Loading