Types removal - deprecate include_type_name with index templates by markharwood · Pull Request #37484 · elastic/elasticsearch

markharwood · 2019-01-15T15:21:06Z

No description provided.

elasticmachine · 2019-01-15T15:21:08Z

Pinging @elastic/es-search

markharwood · 2019-01-16T09:58:14Z

@elasticmachine run elasticsearch-ci-2

markharwood · 2019-01-21T09:14:47Z

IndicesClient.getTemplate is currently failing RestHighLevelClientTests checks on method names matching REST API specs . This is because we are forced to use a non-standard method name while the standard name is occupied by an implementation we are deprecating. Unlike most changes we can't just introduce a method with the same name and change the method arguments because it is only the return type we want to change and differing only by return type would give us a compilation error.
Currently discussing with @hub-cap how we can employ an "interim" name that will pass naming checks until we have passed a deprecation cycle and can adopt the new name. Might require a custom annotation e.g. @InterimName that bypasses strict API naming checks.

jtibshirani

Thanks @markharwood for this work, it is definitely one of the most tricky APIs in the whole group!

I added a few minor suggestions, and also had an overall comment: instead of modifying the server-side request, I I think we should create a new client-side PutIndexTemplateRequest, as we've discussed doing for all the APIs that take include_type_name. This will give better predictability for users, as it will match what we did for put mapping (#37280) and create index (#37134). It also gives a nicer API, since we can start with a fresh object and use method names like source as opposed to untypedSource. Maybe we could even name the new method putIndexTemplate for parity with the new getIndexTemplate?

Currently discussing with @hub-cap how we can employ an "interim" name that will pass naming checks until we have passed a deprecation cycle and can adopt the new name.

Assuming that RestHighLevelClientTests#testApiNamingConventions is test that is failing, could we just add "indices.get_index_template" for now to the list of exceptions here "https://github.com/elastic/elasticsearch/blob/master/client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java#L764-L776 ?

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

client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesClientIT.java

...nt/rest-high-level/src/main/java/org/elasticsearch/client/indices/IndexTemplateMetaData.java

docs/java-rest/high-level/indices/put_template.asciidoc

...er/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetIndexTemplateAction.java

...er/src/main/java/org/elasticsearch/rest/action/admin/indices/RestPutIndexTemplateAction.java

markharwood · 2019-01-22T16:12:23Z

instead of modifying the server-side request, I I think we should create a new client-side PutIndexTemplateRequest,

I can do that but my concerns would be:

duplicate code/maintenance overheads
double standards - TransportClient vs HLRC expectations for types in mappings (eg ML team use the former to register their mappings)
Arguable bugs in the existing server impl - source Vs mapping methods contradictory treatments of typeless mappings. (only the latter "upgrades" typeless content to expected internal typed format).

Are any of these news to you?

could we just add "indices.get_index_template" for now to the list of exceptions here

Done and agreed with Baz.

jtibshirani · 2019-01-22T18:13:34Z

Right, from our discussions we've agreed that the benefits of creating new requests + responses outweigh the downsides. Some specific thoughts:

duplicate code/maintenance overheads

Yes, we are unfortunately creating a bit of duplicate code (as we have been doing in many other places for the HLRC). Longer term, I think @hub-cap is working to mitigate this through an approach where we generating requests + responses.

double standards - TransportClient vs HLRC expectations for types in mappings (eg ML team use the former to register their mappings)

I think this is a relatively short-lived double standard, and only affects internal teams/ users of the deprecated transport client. We'll begin working to remove types from the TransportClient classes in 8.0.

Arguable bugs in the existing server impl - source Vs mapping methods contradictory treatments of typeless mappings. (only the latter "upgrades" typeless content to expected internal typed format).

I've been planning to submit an issue to track + hopefully fix the bug you found in the server requests. I don't really see this point as being related to whether we should create new client classes (and in fact using a new client request may make it simpler to address the bug?)

markharwood · 2019-01-22T18:46:54Z

OK. So we'll deprecate the existing putTemplate with the existing server-side PITR and add a new putTemplate method that takes the new HLRC copy of PITR based on the fixes in this PR.
What's the phasing on this? Presumably 6.7 will have old+new putTemplate methods (old being marked as deprecated) but master will only have the new version.

jtibshirani · 2019-01-22T19:02:26Z

It might be helpful to take a look at the 'put mapping' PR to see the recommended approach (#37280). In the same way that we are accepting both true and false for include_type_name in 7.0, we will actually support both HLRC putTemplate methods in 7.0 as well. The whole PR (deprecation + HLRC changes), can then just be backported to 6.7.

…ntyped APIs

Validate null template name in constructor. Use server side’s GetIndexTemplateResponse class to create XContent for client-side counterpart’s fromXContent test.

* master: (29 commits) Fix limit on retaining sequence number (elastic#37992) Docs test fix, wait for shards active. Revert "Revert "Documented default values for index follow request parameters. (elastic#37917)"" Revert "Documented default values for index follow request parameters. (elastic#37917)" Ensure date parsing BWC compatibility (elastic#37929) SQL: Skip the nested and object field types in case of an ODBC request (elastic#37948) Use mappings to format doc-value fields by default. (elastic#30831) Give precedence to index creation when mixing typed templates with typeless index creation and vice-versa. (elastic#37871) Add classifier to tar.gz in docker compose (elastic#38011) Documented default values for index follow request parameters. (elastic#37917) Fix fetch source option in expand search phase (elastic#37908) Restore a noop _all metadata field for 6x indices (elastic#37808) Added ccr to xpack usage infrastructure (elastic#37256) Fix exit code for Security CLI tools (elastic#37956) Streamline S3 Repository- and Client-Settings (elastic#37393) Add version 6.6.1 (elastic#37975) Ensure task metadata not null in follow test (elastic#37993) Docs fix - missing callout Types removal - deprecate include_type_name with index templates (elastic#37484) Handle completion suggestion without contexts ...

…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

alpar-t · 2019-02-05T15:03:29Z

@markharwood this was reverted in #38427 because it was failing bwc tests on master

markharwood · 2019-02-05T15:50:00Z

Thanks @atorok and sorry for the disruption. Note however that this PR is not the one that was reverted but it was #38022 on 6.x (a modified backport of this PR).

…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

markharwood added WIP :Search Foundations/Mapping Index mappings, including merging and defining field types v7.0.0 labels Jan 15, 2019

markharwood force-pushed the fix/35190TemplateDep branch from 320d119 to 25dee4d Compare January 15, 2019 23:06

markharwood force-pushed the fix/35190TemplateDep branch 3 times, most recently from 4583d6b to 9e6fe7f Compare January 16, 2019 12:45

markharwood mentioned this pull request Jan 16, 2019

Allow indices.get_mapping response parsing without types #37492

Merged

markharwood force-pushed the fix/35190TemplateDep branch 7 times, most recently from 19d1e4b to 69469cb Compare January 17, 2019 14:05

markharwood added the >refactoring label Jan 17, 2019

markharwood force-pushed the fix/35190TemplateDep branch from 909fb6d to 48ab11f Compare January 18, 2019 15:36

jtibshirani reviewed Jan 22, 2019

View reviewed changes

markharwood force-pushed the fix/35190TemplateDep branch from b135f0b to 1082aa2 Compare January 22, 2019 10:30

markharwood removed the WIP label Jan 22, 2019

jtibshirani requested a review from hub-cap January 22, 2019 17:58

This was referenced Jan 23, 2019

Emit a deprecation warning when a typed index template is triggered. #37741

Closed

Implementation tracking for 7.0 types deprecation. #35190

Closed

markharwood added 6 commits January 29, 2019 16:27

Fixed naming convention check, added tests for misuses of typed and u…

2fb874b

…ntyped APIs

Checkstyle fix

5f08782

Fix API naming convention checks

1240b2e

Addressing review comments:

925c064

Validate null template name in constructor. Use server side’s GetIndexTemplateResponse class to create XContent for client-side counterpart’s fromXContent test.

Added test for deprecation warning in RestPutIndexTemplateActionTests

a0326e4

Changed mappings from String to convention of BytesReference

aebbc9b

markharwood force-pushed the fix/35190TemplateDep branch from 1a1d3e0 to aebbc9b Compare January 29, 2019 16:28

markharwood merged commit b889221 into elastic:master Jan 29, 2019

markharwood mentioned this pull request Jan 30, 2019

Backport of types removal for Put/Get index templates #38022

Merged

markharwood added the backport pending label Feb 1, 2019

alpar-t mentioned this pull request Feb 5, 2019

Revert "Backport of types removal for Put/Get index templates (#38022)" #38427

Merged

markharwood mentioned this pull request Feb 5, 2019

Backport of types removal for Put/Get index templates #38465

Merged

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

markharwood removed the backport pending label Feb 7, 2019

Conversation

markharwood commented Jan 15, 2019

Uh oh!

elasticmachine commented Jan 15, 2019

Uh oh!

markharwood commented Jan 16, 2019

Uh oh!

markharwood commented Jan 21, 2019

Uh oh!

jtibshirani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markharwood commented Jan 22, 2019

Uh oh!

jtibshirani commented Jan 22, 2019

Uh oh!

markharwood commented Jan 22, 2019

Uh oh!

jtibshirani commented Jan 22, 2019

Uh oh!

alpar-t commented Feb 5, 2019

Uh oh!

markharwood commented Feb 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants