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.submit to HLRC #53592

Merged
merged 28 commits into from
Mar 19, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
a147c41
Add async_search.submit to HLRC
Mar 12, 2020
ebf5428
iter
Mar 16, 2020
0ed46e6
Change supertype of SubmitAsyncSearchRequest
Mar 16, 2020
701793e
Adding javadoc
Mar 16, 2020
089485b
Overwrite certain SearchRequest paramters on submit request
Mar 16, 2020
87a4f92
Change SearchResponse parsing
Mar 16, 2020
207441d
Merge branch 'master' into async-search-hlrc
Mar 17, 2020
8f708af
Rename response class
Mar 17, 2020
b7d6f14
Don't set CcsMinimizeRountrips in test
Mar 17, 2020
68baeb9
Use setCleanOnCompletion(false) in AsyncSearchIT to always get an ID …
Mar 17, 2020
94083b6
Change ctor of SubmitAsyncSearchRequest
Mar 17, 2020
0796651
Merge branch 'master' into async-search-hlrc
Mar 17, 2020
310fdee
Add request validation and tests
Mar 17, 2020
7e02579
iter on failing test
Mar 17, 2020
c88625a
Expose batchedReduceSize and batchedReduceSize on submit request
Mar 17, 2020
3e5a33f
Adressing review comments
Mar 18, 2020
594a5eb
Remove redundant parameters and validation
Mar 18, 2020
f686a50
Avoid additional boilerplate getters on SubmitAsyncSearchRequest by m…
Mar 18, 2020
1d301c4
Add unsupported parameters to HLRC search request
Mar 18, 2020
4ab7dc7
Revert "Avoid additional boilerplate getters on SubmitAsyncSearchRequ…
Mar 18, 2020
720c966
Add support for max_concurrent_shard_requests
Mar 18, 2020
220e6a8
Merge branch 'master' into async-search-hlrc
Mar 18, 2020
1d0b59c
Merge branch 'master' into async-search-hlrc
Mar 19, 2020
ebd1906
Add docs urls
Mar 19, 2020
f65cf82
Unify conversion of some request parameters
Mar 19, 2020
841430e
adress review comments
Mar 19, 2020
7d3a8ed
Merge branch 'master' into async-search-hlrc
Mar 19, 2020
8223989
iter
Mar 19, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.client;

import org.elasticsearch.action.ActionListener;
import org.elasticsearch.client.asyncsearch.SubmitAsyncSearchRequest;
import org.elasticsearch.client.asyncsearch.SubmitAsyncSearchResponse;

import java.io.IOException;

import static java.util.Collections.emptySet;

public class AsyncSearchClient {
private final RestHighLevelClient restHighLevelClient;

AsyncSearchClient(RestHighLevelClient restHighLevelClient) {
this.restHighLevelClient = restHighLevelClient;
}

// TODO add docs url
cbuescher marked this conversation as resolved.
Show resolved Hide resolved
/**
* Submit a new async search request.
* See <a href="todo"> the docs</a> for more.
* @param request the request
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @return the response
* @throws IOException in case there is a problem sending the request or parsing back the response
*/
public SubmitAsyncSearchResponse submitAsyncSearch(SubmitAsyncSearchRequest request, RequestOptions options) throws IOException {
return restHighLevelClient.performRequestAndParseEntity(request, AsyncSearchRequestConverters::submitAsyncSearch, options,
SubmitAsyncSearchResponse::fromXContent, emptySet());
}

// TODO add docs url
/**
* Asynchronously submit a new async search request.
* See <a href="todo"> the docs</a> for more.
* <a href="https://www.elastic.co/guide/en/elasticsearch/client/java-rest/current/java-rest-high-ilm-ilm-get-lifecycle-policy.html">
* the docs</a> for more.
* @param request the request
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @param listener the listener to be notified upon request completion
* @return cancellable that may be used to cancel the request
*/
public Cancellable submitAsyncSearchAsync(SubmitAsyncSearchRequest request, RequestOptions options,
cbuescher marked this conversation as resolved.
Show resolved Hide resolved
ActionListener<SubmitAsyncSearchResponse> listener) {
return restHighLevelClient.performRequestAsyncAndParseEntity(request, AsyncSearchRequestConverters::submitAsyncSearch, options,
SubmitAsyncSearchResponse::fromXContent, listener, emptySet());
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.client;

import org.apache.http.client.methods.HttpPost;
import org.elasticsearch.action.search.SearchRequest;
import org.elasticsearch.client.RequestConverters.Params;
import org.elasticsearch.client.asyncsearch.SubmitAsyncSearchRequest;

import java.io.IOException;

import static org.elasticsearch.client.RequestConverters.REQUEST_BODY_CONTENT_TYPE;
import static org.elasticsearch.client.RequestConverters.addSearchRequestParams;

final class AsyncSearchRequestConverters {

static Request submitAsyncSearch(SubmitAsyncSearchRequest asyncSearchRequest) throws IOException {
SearchRequest searchRequest = asyncSearchRequest.getSearchRequest();
String endpoint = new RequestConverters.EndpointBuilder().addCommaSeparatedPathParts(
searchRequest.indices())
.addPathPartAsIs("_async_search").build();
Request request = new Request(HttpPost.METHOD_NAME, endpoint);
Params params = new RequestConverters.Params();
// add all typical search params and search request source as body
addSearchRequestParams(params, searchRequest);
if (searchRequest.source() != null) {
request.setEntity(RequestConverters.createEntity(searchRequest.source(), REQUEST_BODY_CONTENT_TYPE));
}
// set async search submit specific parameters
if (asyncSearchRequest.isCleanOnCompletion() != null) {
params.putParam("clean_on_completion", asyncSearchRequest.isCleanOnCompletion().toString());
}
if (asyncSearchRequest.getKeepAlive() != null) {
params.putParam("keep_alive", asyncSearchRequest.getKeepAlive().getStringRep());
}
if (asyncSearchRequest.getWaitForCompletion() != null) {
params.putParam("wait_for_completion", asyncSearchRequest.getWaitForCompletion().getStringRep());
}
request.addParameters(params.asMap());
return request;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ static Request search(SearchRequest searchRequest, String searchEndpoint) throws
return request;
}

private static void addSearchRequestParams(Params params, SearchRequest searchRequest) {
static void addSearchRequestParams(Params params, SearchRequest searchRequest) {
params.putParam(RestSearchAction.TYPED_KEYS_PARAM, "true");
params.withRouting(searchRequest.routing());
params.withPreference(searchRequest.preference());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,7 @@ public class RestHighLevelClient implements Closeable {
private final TransformClient transformClient = new TransformClient(this);
private final EnrichClient enrichClient = new EnrichClient(this);
private final EqlClient eqlClient = new EqlClient(this);
private final AsyncSearchClient asyncSearchClient = new AsyncSearchClient(this);

/**
* Creates a {@link RestHighLevelClient} given the low level {@link RestClientBuilder} that allows to build the
Expand Down Expand Up @@ -428,13 +429,22 @@ public final XPackClient xpack() {
* A wrapper for the {@link RestHighLevelClient} that provides methods for
* accessing the Elastic Index Lifecycle APIs.
* <p>
* See the <a href="http://FILL-ME-IN-WE-HAVE-NO-DOCS-YET.com"> X-Pack APIs
* See the <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/index-lifecycle-management-api.html"> X-Pack APIs
* on elastic.co</a> for more information.
*/
public IndexLifecycleClient indexLifecycle() {
return ilmClient;
}

/**
* A wrapper for the {@link RestHighLevelClient} that provides methods for accessing the Elastic Index Async Search APIs.
* <p>
* See the <a href="http://FILL-ME-IN-WE-HAVE-NO-DOCS-YET.com"> X-Pack APIs on elastic.co</a> for more information.
*/
public AsyncSearchClient asyncSearch() {
return asyncSearchClient;
}

/**
* Provides methods for accessing the Elastic Licensed Migration APIs that
* are shipped with the default distribution of Elasticsearch. All of
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/


package org.elasticsearch.client.asyncsearch;

import org.elasticsearch.action.search.SearchRequest;
import org.elasticsearch.client.TimedRequest;
import org.elasticsearch.common.unit.TimeValue;

import java.util.Objects;

public class SubmitAsyncSearchRequest extends TimedRequest {
cbuescher marked this conversation as resolved.
Show resolved Hide resolved

private TimeValue waitForCompletion;
private Boolean cleanOnCompletion;
private TimeValue keepAlive;
private final SearchRequest searchRequest;

public SubmitAsyncSearchRequest(SearchRequest searchRequest) {
cbuescher marked this conversation as resolved.
Show resolved Hide resolved
this.searchRequest = searchRequest;
}

cbuescher marked this conversation as resolved.
Show resolved Hide resolved
public SearchRequest getSearchRequest() {
Copy link
Member

Choose a reason for hiding this comment

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

I see that whether or not to expose the inner search request was discussed in previous reviews. I am confused though on what direction was chosen. I see that the getters and setters from the search request are copied to the async request, but the inner search request is still exposed through this getter and can be modified directly. Didn't we want to rather hide it from users?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to hide

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I will look into that. Will make testing a bit more awkward though because I can't reuse some existing infra then.

Copy link
Contributor

Choose a reason for hiding this comment

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

package protected ? ;)

Copy link
Member Author

@cbuescher cbuescher Mar 18, 2020

Choose a reason for hiding this comment

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

Would work but then I need to move some classes around. I'd prefer that to adding all those getters to the submit request.

Copy link
Member Author

@cbuescher cbuescher Mar 18, 2020

Choose a reason for hiding this comment

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

package protected ? ;)

This is proving to be tricky. I cannot easily move the AsyncSearchRequestConverters out of the org.elasticsearch.client package since they need package private infra from RequestConverters. I can probably move the SubmitAsyncSearchRequest into that package though, will take a look what that would change.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed f686a50 which moves the new request and response classes into org.elasticsearch.client to be able to use a package protected getSearchRequest() to avoid all the boilerplate getters on the new request. I'm unsure what I like best, take a look and let me know which direction you are leaning.

Copy link
Member

Choose a reason for hiding this comment

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

I think no getter is safer. We also do have extra protection on the server side for unsupported values but I think the client should never allow to send unsupported values.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Luca, we should avoid the getter on the search request. However, we should have a getter for all options that we expose so I don't see why you want to avoid them ? If we have a setter, we need to provide a way to access the value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I reverted that change back to the one where I removed getSearchRequest and added a bunch of getters.

return searchRequest;
}

public TimeValue getWaitForCompletion() {
return waitForCompletion;
}

public void setWaitForCompletion(TimeValue waitForCompletion) {
this.waitForCompletion = waitForCompletion;
}

public Boolean isCleanOnCompletion() {
return cleanOnCompletion;
}

public void setCleanOnCompletion(boolean cleanOnCompletion) {
this.cleanOnCompletion = cleanOnCompletion;
}

public TimeValue getKeepAlive() {
return keepAlive;
}

public void setKeepAlive(TimeValue keepAlive) {
this.keepAlive = keepAlive;
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
SubmitAsyncSearchRequest request = (SubmitAsyncSearchRequest) o;
return Objects.equals(getSearchRequest(), request.getSearchRequest())
&& Objects.equals(getKeepAlive(), request.getKeepAlive())
&& Objects.equals(getWaitForCompletion(), request.getWaitForCompletion())
&& Objects.equals(isCleanOnCompletion(), request.isCleanOnCompletion());
}

@Override
public int hashCode() {
return Objects.hash(getSearchRequest(), getKeepAlive(), getWaitForCompletion(), isCleanOnCompletion());
}
}
Loading