Standardize underscore requirements in parameters #27040

mayya-sharipova · 2017-10-18T09:28:36Z

Stardardize underscore requirements in parameters across different type of
requests:
_index, _type, _source, _id keep their underscores
params like version and retry_on_conflict will be without underscores

BulkRequest, MultiGetRequest, TermVectorcRequest, MoreLikeThisQuery were
changed

Closes #26886

nik9000

Makes sense to me but I think it needs breaking changes docs before we can merge it.

I also would have expect some of the documentation tests and yaml tests to fail. If none fail that means we aren't testing this stuff too well.

I also think that there are some tables in the documentation that probably need updating. Like here, for example.

nik9000 · 2017-10-18T13:29:53Z

core/src/main/java/org/elasticsearch/action/get/MultiGetRequest.java

+    private static final ParseField INDEX = new ParseField("_index");
+    private static final ParseField TYPE = new ParseField("_type");
+    private static final ParseField ID = new ParseField("_id");
+    private static final ParseField ROUTING = new ParseField("routing", "_routing");


If these are common I wonder if we can put them in a common place.

nik9000 · 2017-10-18T13:32:25Z

core/src/main/java/org/elasticsearch/action/termvectors/TermVectorsRequest.java

+    private static final ParseField ROUTING = new ParseField("routing", "_routing");
+    private static final ParseField PARENT = new ParseField("parent", "_parent");
+    private static final ParseField VERSION = new ParseField("version", "_version");
+    private static final ParseField VERSION_TYPE = new ParseField("version_type", "_version_type", "_versionType", "versionType");


We've dropped support for camel case most other places a while ago. I can see that we kept it here. This change will start to emit a deprecation warning which is great. I'm fine with keeping this camel case for another major version and dropping it then. It might be good to add a test that tries to use the deprecated camel case that fails when the current version's major is > 7. Then when we bump the version we'll drop the camel case.

nik9000 · 2017-10-18T13:33:11Z

core/src/main/java/org/elasticsearch/index/query/MoreLikeThisQueryBuilder.java

@@ -148,9 +148,10 @@
            ParseField DOC = new ParseField("doc");
            ParseField FIELDS = new ParseField("fields");
            ParseField PER_FIELD_ANALYZER = new ParseField("per_field_analyzer");
-            ParseField ROUTING = new ParseField("_routing");


We've mostly been removing these Field interfaces as we go. Could you remove it here too?

nik9000 · 2017-10-18T13:42:23Z

On second look this isn't actually breaking because it doesn't change any responses. I still think it should have notes in the breaking changes documentation about deprecating the parameters, but only in the 6.x branch. Since we'll only write them when backporting I withdraw my request to add them to this PR. I still think it is worth updating the docs though.

olcbean · 2017-10-26T17:07:50Z

@mayya-sharipova @nik9000
I remember that there is an older issue about something similar.
Would this PR also fix #13377?

mayya-sharipova · 2017-10-26T17:55:58Z

@olcbean thanks for bringing this up. I think it should fix #13377 as well. As Nik commented, in 7.X camelCase, and underscore params will produce deprecated warnings, but in 8.x we plan to throw errors for cameCase.

jimczi

Thanks @mayya-sharipova
The change looks good.
Can you add a deprecation test for the bulk and multi_get ?
Also I wonder if we can remove all the _ and camel case versions in 7 and deprecate in 6.1 ? @clintongormley do you think that 6.x is enough for the deprecation period or we should only remove in 8 ?

jimczi · 2017-11-03T11:40:02Z

rest-api-spec/src/main/resources/rest-api-spec/test/mtermvectors/20_deprecated.yml

+"Deprecated camel case parameters should fail in Term Vectors query":
+
+   - skip:
+       version: " - 7.99.99"


I don't think we've done that before. I understand the intent but I am not sure we should do that. Maybe add a note in the comments about deprecation and removal ? Blocking a version bump with a test is a bit too much IMO.

@nik9000 just saw that you asked for this test. Should we really do that ? If we start adding expectations in the tests for future versions it might take a while to just be able to bump the version.

@jimczi This is okay, we do do this as an easy way to track code that needs to be removed in the future (see #27216 for another example). Yes, it does mean that bumping the version to 8.0.0 will take a little more time but I think that's outweighed by the benefit of being able to remove code that should be dead. As long as the assertion/test is covered with a clear comment about what should be removed, I think these removals should be relatively straightforward.

Fair enough. Thanks for explaining @jasontedor . If it's contained and easy to remove I agree that it's a good way to ensure that we do remove the code in the next major version. Let's try to not use this for something else than dead code ;) @mayya-sharipova can you add a note in the reason regarding what needs to be done when the version is bumped.

clintongormley · 2017-11-03T11:59:07Z

@clintongormley do you think that 6.x is enough for the deprecation period or we should only remove in 8 ?

I think deprecation in 6.x and removal in 7.0 will be fine.

mayya-sharipova · 2017-11-05T13:36:52Z

retest this please

mayya-sharipova · 2017-11-06T00:25:26Z

retest this please

Stardardize underscore requirements in parameters across different type of requests: _index, _type, _source, _id keep their underscores params like version and retry_on_conflict will be without underscores BulkRequest, MultiGetRequest, TermVectorcRequest, MoreLikeThisQuery were changed Closes elastic#26886

Stardardize underscore requirements in parameters across different type of requests: _index, _type, _source, _id keep their underscores params like version and retry_on_conflict will be without underscores In 6.x these parameters are deprecated and produce deprecated warnings. BulkRequest, MultiGetRequest, TermVectorcRequest, MoreLikeThisQuery were changed Closes elastic#26886

mayya-sharipova · 2017-11-08T11:52:05Z

retest this please

…pe of requests: _index, _type, _source, _id keep their underscores params like version and retry_on_conflict will be without underscores Throw an error if older versions of parameters are used BulkRequest, MultiGetRequest, TermVectorcRequest, MoreLikeThisQuery were changed Related to elastic#27040 Closes elastic#26886

nik9000 · 2018-03-12T21:23:25Z

rest-api-spec/src/main/resources/rest-api-spec/test/mget/80_deprecated.yml

+
+---
+"Deprecated parameters should produce warning in Multi Get query2":
+# MODIFY in 7.x as these throw errors instead of warnings


In master these should still work, right? Until we make a followup that removes support for the camelCase and stuff.

mayya-sharipova · 2018-03-14T16:31:41Z

Closing, because of a duplicate: #27414 which is already merged to master

nik9000 requested changes Oct 18, 2017

View reviewed changes

nik9000 added :Core/Infra/REST API REST infrastructure and utilities >breaking v7.0.0 v6.1.0 and removed >breaking labels Oct 18, 2017

mayya-sharipova force-pushed the enhancement/standardize-underscore-parameters branch 2 times, most recently from 92e9800 to 27bb0e7 Compare October 25, 2017 18:40

mayya-sharipova force-pushed the enhancement/standardize-underscore-parameters branch from 27bb0e7 to ceed274 Compare October 27, 2017 11:10

mayya-sharipova mentioned this pull request Nov 1, 2017

Add limits for ngram and shingle settings #27211

Merged

jimczi requested changes Nov 3, 2017

View reviewed changes

mayya-sharipova force-pushed the enhancement/standardize-underscore-parameters branch from 8a27718 to c050719 Compare November 4, 2017 21:07

mayya-sharipova force-pushed the enhancement/standardize-underscore-parameters branch from c050719 to 6032f04 Compare November 6, 2017 13:34

mayya-sharipova added 3 commits November 7, 2017 18:23

mayya-sharipova force-pushed the enhancement/standardize-underscore-parameters branch from 926005a to b1c4450 Compare November 7, 2017 23:30

mayya-sharipova mentioned this pull request Nov 16, 2017

Standardize underscore requirements in parameters #27414

Merged

lcawl added v6.2.0 and removed v6.1.0 labels Dec 12, 2017

colings86 added the v6.3.0 label Jan 22, 2018

colings86 removed the v6.2.0 label Jan 22, 2018

nik9000 approved these changes Mar 12, 2018

View reviewed changes

mayya-sharipova closed this Mar 14, 2018

mayya-sharipova deleted the enhancement/standardize-underscore-parameters branch March 14, 2018 16:31

jpountz added the >enhancement label Jun 13, 2018

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

mariha mentioned this pull request Jun 4, 2019

Use only one way of specifying routing, version and version_type #13377

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize underscore requirements in parameters #27040

Standardize underscore requirements in parameters #27040

mayya-sharipova commented Oct 18, 2017 •

edited by nik9000

Loading

nik9000 left a comment

nik9000 Oct 18, 2017

nik9000 Oct 18, 2017

nik9000 Oct 18, 2017

nik9000 commented Oct 18, 2017

olcbean commented Oct 26, 2017

mayya-sharipova commented Oct 26, 2017

jimczi left a comment

jimczi Nov 3, 2017

jimczi Nov 3, 2017

jasontedor Nov 3, 2017

jimczi Nov 3, 2017

clintongormley commented Nov 3, 2017

mayya-sharipova commented Nov 5, 2017

mayya-sharipova commented Nov 6, 2017

mayya-sharipova commented Nov 8, 2017

nik9000 Mar 12, 2018

mayya-sharipova commented Mar 14, 2018 •

edited

Loading

Standardize underscore requirements in parameters #27040

Standardize underscore requirements in parameters #27040

Conversation

mayya-sharipova commented Oct 18, 2017 • edited by nik9000 Loading

nik9000 left a 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

nik9000 commented Oct 18, 2017

olcbean commented Oct 26, 2017

mayya-sharipova commented Oct 26, 2017

jimczi left a 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

Choose a reason for hiding this comment

clintongormley commented Nov 3, 2017

mayya-sharipova commented Nov 5, 2017

mayya-sharipova commented Nov 6, 2017

mayya-sharipova commented Nov 8, 2017

Choose a reason for hiding this comment

mayya-sharipova commented Mar 14, 2018 • edited Loading

mayya-sharipova commented Oct 18, 2017 •

edited by nik9000

Loading

mayya-sharipova commented Mar 14, 2018 •

edited

Loading