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

Standardize underscore requirements in parameters #27040

Conversation

@mayya-sharipova
Copy link
Contributor

mayya-sharipova commented Oct 18, 2017

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

Copy link
Contributor

nik9000 left a comment

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.

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");

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 18, 2017

Contributor

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

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");

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 18, 2017

Contributor

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.

core/src/main/java/org/elasticsearch/index/query/MoreLikeThisQueryBuilder.java Outdated
@@ -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");

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 18, 2017

Contributor

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

@nik9000

This comment has been minimized.

Copy link
Contributor

nik9000 commented Oct 18, 2017

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.

@mayya-sharipova mayya-sharipova force-pushed the mayya-sharipova:enhancement/standardize-underscore-parameters branch 2 times, most recently Oct 24, 2017
@olcbean

This comment has been minimized.

Copy link
Contributor

olcbean commented Oct 26, 2017

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

@mayya-sharipova

This comment has been minimized.

Copy link
Contributor Author

mayya-sharipova commented Oct 26, 2017

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

@mayya-sharipova mayya-sharipova force-pushed the mayya-sharipova:enhancement/standardize-underscore-parameters branch Oct 27, 2017
Copy link
Member

jimczi left a comment

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 ?

rest-api-spec/src/main/resources/rest-api-spec/test/mtermvectors/20_deprecated.yml Outdated
"Deprecated camel case parameters should fail in Term Vectors query":

- skip:
version: " - 7.99.99"

This comment has been minimized.

Copy link
@jimczi

jimczi Nov 3, 2017

Member

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.

This comment has been minimized.

Copy link
@jimczi

jimczi Nov 3, 2017

Member

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

This comment has been minimized.

Copy link
@jasontedor

jasontedor Nov 3, 2017

Member

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

This comment has been minimized.

Copy link
@jimczi

jimczi Nov 3, 2017

Member

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

This comment has been minimized.

Copy link
Member

clintongormley commented Nov 3, 2017

@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 mayya-sharipova force-pushed the mayya-sharipova:enhancement/standardize-underscore-parameters branch Nov 4, 2017
@mayya-sharipova

This comment has been minimized.

Copy link
Contributor Author

mayya-sharipova commented Nov 5, 2017

retest this please

1 similar comment
@mayya-sharipova

This comment has been minimized.

Copy link
Contributor Author

mayya-sharipova commented Nov 6, 2017

retest this please

@mayya-sharipova mayya-sharipova force-pushed the mayya-sharipova:enhancement/standardize-underscore-parameters branch Nov 6, 2017
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
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
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 #26886
@mayya-sharipova mayya-sharipova force-pushed the mayya-sharipova:enhancement/standardize-underscore-parameters branch to b1c4450 Nov 7, 2017
@mayya-sharipova

This comment has been minimized.

Copy link
Contributor Author

mayya-sharipova commented Nov 8, 2017

retest this please

mayya-sharipova added a commit to mayya-sharipova/elasticsearch that referenced this pull request Nov 17, 2017
…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
@lcawl lcawl added v6.2.0 and removed v6.1.0 labels Dec 12, 2017
@colings86 colings86 added the v6.3.0 label Jan 22, 2018
@colings86 colings86 removed the v6.2.0 label Jan 22, 2018

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

This comment has been minimized.

Copy link
@nik9000

nik9000 Mar 12, 2018

Contributor

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

@mayya-sharipova

This comment has been minimized.

Copy link
Contributor Author

mayya-sharipova commented Mar 14, 2018

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

@mayya-sharipova mayya-sharipova deleted the mayya-sharipova:enhancement/standardize-underscore-parameters branch Mar 14, 2018
@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.