-
Notifications
You must be signed in to change notification settings - Fork 1.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
[2.x] Remove deprecation warning of using REST API request parameter 'master_timeout' #2920
[2.x] Remove deprecation warning of using REST API request parameter 'master_timeout' #2920
Conversation
❌ Gradle Check failure 45f31149c1c2b6b9e60c8631345d8dc6c97de75e |
…r_timeout' Signed-off-by: Tianli Feng <ftianli@amazon.com>
…r_timeout' - comment out part Signed-off-by: Tianli Feng <ftianli@amazon.com>
45f3114
to
fb59fcf
Compare
Signed-off-by: Tianli Feng <ftianli@amazon.com>
…n 2 action classes Signed-off-by: Tianli Feng <ftianli@amazon.com>
@@ -203,27 +202,19 @@ protected Set<String> responseParams() { | |||
return Collections.emptySet(); | |||
} | |||
|
|||
protected static final String DUPLICATE_PARAMETER_ERROR_MESSAGE = | |||
"Please only use one of the request parameters [master_timeout, cluster_manager_timeout]."; | |||
|
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.
Note: I moved the constant String outside the method to make it directly used in RestIndicesAction and RestAllocationAction classes in below change. In these 2 classes, the method parseDeprecatedMasterTimeoutParameter()
can not fit the requirement, so need the error message used outside the BaseRestHandler
, directly in the 2 classes.
if (request.hasParam("master_timeout")) { | ||
logger.deprecate(logMsgKeyPrefix + "_master_timeout_parameter", MASTER_TIMEOUT_DEPRECATED_MESSAGE); | ||
if (request.hasParam("cluster_manager_timeout")) { | ||
throw new OpenSearchParseException(DUPLICATE_PARAMETER_ERROR_MESSAGE); | ||
} |
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.
Note: the method arguments logger
and logMsgKeyPrefix
are removed since they are not used without deprecation warning.
…'master_timeout' (#2920) The plan of deprecation parameter "master_timeout" changed to not show deprecation warning of 'master_timeout' parameter in 2.x version, but start to emit warning from version 3.0 . For the detailed plan, see issue #2928. Signed-off-by: Tianli Feng <ftianli@amazon.com> (cherry picked from commit 8bc5765)
Description
The REST API request parameter 'master_timeout' is deprecated in issue #2511.
The PR removes the deprecation warning when assigning value to the parameter 'master_timeout'.
The plan of deprecation parameter "master_timeout" changed to not show deprecation warning of 'master_timeout' parameter in 2.x version, but start to emit warning from version 3.0 .
For the detailed plan, see issue #2928.
Reason:
This is a compromise with the High Level REST Client (https://github.com/opensearch-project/OpenSearch/tree/1.3.1/client/rest-high-level).
In High Level REST Client, parameter 'master_timeout' is added to every applicable REST API call,
see org.opensearch.client.RequestConverters.Params.withMasterTimeout(TimeValue).
To keep the compatibility of Rest Client 2.x with server 1.x, the parameter 'master_timeout' is preserved.
Emitting deprecation warning in log file and HTTP response header in this case will confuse the user, because the deprecated parameter is not actively used by the user.
Issues Resolved
A part of issue #2928 and #2511
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.