Types removal - deprecate include_type_name with index templates

[markharwood]

No description provided.

[elasticmachine]

Pinging @elastic/es-search

[markharwood]

@elasticmachine run elasticsearch-ci-2

[markharwood]

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 ?

[markharwood]

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:

1. duplicate code/maintenance overheads
2. double standards - TransportClient vs HLRC expectations for types in mappings (eg ML team use the former to register their mappings)
3. 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]

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]

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]

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.

[alpar-t]

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

[markharwood]

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).