-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Add high-level client methods that accept RequestOptions #31069
Add high-level client methods that accept RequestOptions #31069
Conversation
With elastic#30490 we have introduced a new way to provide request options whenever sending a request using the high-level REST client. Before you could provide headers as the last argument varargs of each API method, now you can provide `RequestOptions` that in the future will allow to provide more options which can be specified per request. This commit deprecates all of the client methods that accept a `Header` varargs argument in favour of new methods that accept `RequestOptions` instead. For some API we don't even go through deprecation given that they were not released since they were added, hence in that case we can just move them to the new method.
Pinging @elastic/es-core-infra |
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.
Few small changes.
* <p> | ||
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-flush.html"> Flush API on elastic.co</a> | ||
* @deprecated Prefer {@link #flushAsync(FlushRequest, RequestOptions, ActionListener)} | ||
*/ |
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.
missed a @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.
this I had already fixed like other things, but I forgot to push my latest commit :)
* <p> | ||
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-update-settings.html"> Update Indices Settings | ||
* API on elastic.co</a> | ||
* @deprecated Prefer {@link #putMappingAsync(PutMappingRequest, RequestOptions, ActionListener)} |
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.
s/putMappingAsync/putSettingsAsync
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.
wow that's a good catch :) thanks for finding it.
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 left a minor thing. Other than @hub-cap's thing I think it is great! Thanks for doing this. I was going to start on it after the node selector business but you got it!
I think we'll want to do some documentation changes after this but I think you or I should do it in a followup.
@@ -146,6 +224,20 @@ public void putMappingAsync(PutMappingRequest putMappingRequest, ActionListener< | |||
* "https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-aliases.html"> | |||
* Index Aliases API on elastic.co</a> |
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 think it'd be nice to add an @param options
to the Javadoc to describe this and to point folks to RequestOptions.DEFAULTS
.
def +1 to this being better than the |
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.
not sure why i -1'd the changes, this looks fine sans the nits.
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/cluster-update-settings.html"> Cluster Update Settings | ||
* API on elastic.co</a> | ||
* @param clusterUpdateSettingsRequest the request | ||
* @param options the request options (e.g. headers), or {@link RequestOptions#DEFAULT} if nothing needs to be customized |
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.
s/or/use/ ?
* Updates cluster wide specific settings using the Cluster Update Settings API. | ||
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/cluster-update-settings.html"> Cluster Update Settings | ||
* API on elastic.co</a> | ||
* @param clusterUpdateSettingsRequest the request |
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'm not really sure this needs a param tag but it doesn't hurt anything.
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 went for having all the param tags rather than only some of them
* API on elastic.co</a> | ||
* @param clusterUpdateSettingsRequest the request | ||
* @param options the request options (e.g. headers), or {@link RequestOptions#DEFAULT} if nothing needs to be customized | ||
* @return the response |
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 don't think the@return
tag adds anything here. I think it is fairly obvious from the type of the response.
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.
agreed, I added them for consistency reasons, given that we added param tags it felt weird not to have also the return one.
This PR removes all the API methods that accept a `Header` varargs argument, in favour of the newly introduced API methods that accept a `RequestOptions` argument. Relates to elastic#31069
With #30490 we have introduced a new way to provide request options whenever sending a request using the high-level REST client. Before you could provide headers as the last argument varargs of each API method, now you can provide `RequestOptions` that in the future will allow to provide more options which can be specified per request. This commit deprecates all of the client methods that accept a `Header` varargs argument in favour of new methods that accept `RequestOptions` instead. For some API we don't even go through deprecation given that they were not released since they were added, hence in that case we can just move them to the new method.
* 6.x: Move default location of dependencies report (#31228) Remove dependencies report task dependencies (#31227) Add recognition of MPL 2.0 (#31226) Fix unknown licenses (#31223) Fully encapsulate LocalCheckpointTracker inside of the engine (#31213) Remove version from license file name for GCS SDK (#31221) Remove DocumentFieldMappers#simpleMatchToFullName. (#31041) [DOCS] Removes 6.3.1 release notes [DOCS] Splits release notes by major version Remove DocumentFieldMappers#smartNameFieldMapper, as it is no longer needed. (#31018) Remove extraneous references to 'tokenized' in the mapper code. (#31010) SQL: Make a single JDBC driver jar (#31012) QA: Fix rolling restart tests some more Allow to trim all ops above a certain seq# with a term lower than X high level REST api: cancel task (#30745) Mute TokenBackwardsCompatibilityIT.testMixedCluster Mute WatchBackwardsCompatibilityIT.testWatcherRestart Enhance license detection for various licenses (#31198) [DOCS] Add note about long-lived idle connections (#30990) Add high-level client methods that accept RequestOptions (#31069) Remove RestGetAllMappingsAction (#31129) Move RestGetSettingsAction to RestToXContentListener (#31101) Move number of language analyzers to analysis-common module (#31143) flush job to ensure all results have been written (#31187)
This commit removes all the API methods that accept a `Header` varargs argument, in favour of the newly introduced API methods that accept a `RequestOptions` argument. Relates to #31069
With #30490 we have introduced a new way to provide request options
whenever sending a request using the high-level REST client. Before you
could provide headers as the last argument varargs of each API method,
now you can provide
RequestOptions
that in the future will allow toprovide more options which can be specified per request.
This commit deprecates all of the client methods that accept a
Header
varargs argument in favour of new methods that accept
RequestOptions
instead. For some API we don't even go through deprecation given that
they were not released since they were added, hence in that case we can
just move them to the new method.