-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Deprecate _upgrade
API (#47678)
#50484
Conversation
984c704
to
8c83273
Compare
Pinging @elastic/es-core-features (:Core/Features/Java High Level REST Client) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this @timoninmaxim, I left a couple of comments. I don't think we need to rename the classes here, just annotate them with @Deprecated
...r/src/main/java/org/elasticsearch/rest/action/admin/indices/RestUpgradeActionDeprecated.java
Show resolved
Hide resolved
...main/java/org/elasticsearch/rest/action/admin/indices/RestUpgradeStatusActionDeprecated.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@elasticmachine ok to test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there are some test failures as a result of this change.
@@ -47,6 +47,10 @@ | |||
"default":"open", | |||
"description":"Whether to expand wildcard expression to concrete indices that are open, closed or both." | |||
} | |||
}, | |||
"deprecated":{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that this field causes all sorts of test failures. I believe only the "parts"
sub-objects support deprecations in the REST spec for now, so this should be removed and "DEPRECATED" added to the description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my fault, I used wrong gradle check
command and missed this tests. So, I've fixed it and some other tests in new commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used this doc for deprecation: https://github.com/elastic/elasticsearch/tree/master/rest-api-spec#deprecations. Currenlty it incorrectly shows how to deprecate entire API. Is it a inaccuracy in the documentation or bug in a code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @dakrone ! Could you please check my last comment above? Should we do smth with that the doc is incorrect?
@@ -55,6 +55,10 @@ | |||
"type":"boolean", | |||
"description":"If true, only ancient (an older Lucene major release) segments will be upgraded" | |||
} | |||
}, | |||
"deprecated":{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about deprecation here
defdf38
to
76bd135
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for iterating on this @timoninmaxim! |
* Deprecates _upgrade API Ref elastic#47678 * Move deprecation flags to path section. Add deprecation warning tests for _upgrade API. Ref elastic#47678
* Deprecates _upgrade API Ref elastic#47678 * Move deprecation flags to path section. Add deprecation warning tests for _upgrade API. Ref elastic#47678
_upgrade
API (#47678)
Issue comment #47678
Suggest deprecate Upgrade API instead of implementing high-level API for it.