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

Add async_search get and delete APIs to HLRC #53828

Merged
merged 7 commits into from
Mar 23, 2020

Conversation

cbuescher
Copy link
Member

This commit adds the "_async_searhc" get and delete APIs to the
AsyncSearchClient in the High Level Rest Client.

Relates to #49091

@cbuescher cbuescher added :Search/Search Search-related issues that do not fall into other categories :Core/Features/Java High Level REST Client v8.0.0 v7.7.0 labels Mar 19, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/Java High Level REST Client)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

This commit adds the "_async_searhc" get and delete APIs to the
AsyncSearchClient in the High Level Rest Client.

Relates to elastic#49091
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left one comment regarding the naming in the AsyncSearchClient, LGTM otherwise.

* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/async-search.html"> the docs</a> for more.
*
*/
public AsyncSearchResponse getAsyncSearch(GetAsyncSearchRequest request, RequestOptions options) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can rename into get and getAsync since we're already inside the AsyncSearchClient ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, no problem with that...

* @return the response
* @throws IOException in case there is a problem sending the request or parsing back the response
*/
public AcknowledgedResponse deleteAsyncSearch(DeleteAsyncSearchRequest request, RequestOptions options) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, delete should be enough ?

* @param listener the listener to be notified upon request completion
* @return cancellable that may be used to cancel the request
*/
public Cancellable deleteAsyncSearchAsync(DeleteAsyncSearchRequest request, RequestOptions options,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleteAsync ?

return restHighLevelClient.performRequestAsyncAndParseEntity(request, AsyncSearchRequestConverters::deleteAsyncSearch, options,
AcknowledgedResponse::fromXContent, listener, emptySet());
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also rename submitAsyncSearch into submit and submitAsync ?

@cbuescher cbuescher merged commit 3ceb60b into elastic:master Mar 23, 2020
cbuescher pushed a commit to cbuescher/elasticsearch that referenced this pull request Mar 23, 2020
This commit adds the "_async_searhc" get and delete APIs to the
AsyncSearchClient in the High Level Rest Client.

Relates to elastic#49091
Backport of elastic#53828
cbuescher pushed a commit that referenced this pull request Mar 23, 2020
This commit adds the "_async_searhc" get and delete APIs to the
AsyncSearchClient in the High Level Rest Client.

Relates to #49091
Backport of #53828
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Search/Search Search-related issues that do not fall into other categories v7.7.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants