Standardize underscore requirements in parameters#27040
Standardize underscore requirements in parameters#27040mayya-sharipova wants to merge 3 commits intoelastic:masterfrom
Conversation
nik9000
left a comment
There was a problem hiding this 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.
There was a problem hiding this comment.
If these are common I wonder if we can put them in a common place.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We've mostly been removing these Field interfaces as we go. Could you remove it here too?
|
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. |
92e9800 to
27bb0e7
Compare
|
@mayya-sharipova @nik9000 |
27bb0e7 to
ceed274
Compare
jimczi
left a comment
There was a problem hiding this 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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
I think deprecation in 6.x and removal in 7.0 will be fine. |
8a27718 to
c050719
Compare
|
retest this please |
1 similar comment
|
retest this please |
c050719 to
6032f04
Compare
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 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
926005a to
b1c4450
Compare
|
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
|
|
||
| --- | ||
| "Deprecated parameters should produce warning in Multi Get query2": | ||
| # MODIFY in 7.x as these throw errors instead of warnings |
There was a problem hiding this comment.
In master these should still work, right? Until we make a followup that removes support for the camelCase and stuff.
|
Closing, because of a duplicate: #27414 which is already merged to master |
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