Add async_search.submit to HLRC #53592

cbuescher · 2020-03-16T10:54:20Z

This commit adds a new AsyncSearchClient to the High Level Rest Client which
initially supporst the submitAsyncSearch in its blocking and non-blocking
flavour. Also adding client side request and response objects and parsing code
to parse the xContent output of the client side AsyncSearchResponse together
with parsing roundtrip tests and a simple roundtrip integration test.

Relates to #49091

This commit adds a new AsyncSearchClient to the High Level Rest Client which initially supporst the submitAsyncSearch in its blocking and non-blocking flavour. Also adding client side request and response objects and parsing code to parse the xContent output of the client side AsyncSearchResponse together with parsing roundtrip tests and a simple roundtrip integration test. Relates to elastic#49091

elasticmachine · 2020-03-16T10:54:22Z

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

elasticmachine · 2020-03-16T10:54:23Z

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

client/rest-high-level/src/test/java/org/elasticsearch/client/asyncsearch/AsyncSearchIT.java

jimczi

The change looks good to me. I left some comments but @javanna may want to take a look too.

...-high-level/src/main/java/org/elasticsearch/client/asyncsearch/SubmitAsyncSearchRequest.java

server/src/main/java/org/elasticsearch/action/search/SearchResponse.java

...high-level/src/main/java/org/elasticsearch/client/asyncsearch/SubmitAsyncSearchResponse.java

...-high-level/src/main/java/org/elasticsearch/client/asyncsearch/SubmitAsyncSearchRequest.java

...high-level/src/main/java/org/elasticsearch/client/asyncsearch/SubmitAsyncSearchResponse.java

client/rest-high-level/src/test/java/org/elasticsearch/client/asyncsearch/AsyncSearchIT.java

…back

cbuescher · 2020-03-17T10:26:03Z

@jimczi thanks for the review, I pushed some updates and left a reply regarding changing the client-side SubmitAsyncSearchRequest constructor, let me know what you think.

cbuescher · 2020-03-17T14:59:32Z

I pushed a change with a different ctor, I'm still not a fan of this but added it as a base for discussion.

jimczi

I like the new ctr better, I also find it cleaner to not expose the raw search request. Can you explain why you're not happy with it ?

...-high-level/src/main/java/org/elasticsearch/client/asyncsearch/SubmitAsyncSearchRequest.java

javanna

I left a couple of comments but nothing major besides a question on exposing the inner search request. Thanks for picking this up!!!

client/rest-high-level/src/main/java/org/elasticsearch/client/AsyncSearchClient.java

client/rest-high-level/src/main/java/org/elasticsearch/client/RequestConverters.java

.../rest-high-level/src/main/java/org/elasticsearch/client/asyncsearch/AsyncSearchResponse.java

javanna · 2020-03-18T10:18:25Z

...-high-level/src/main/java/org/elasticsearch/client/asyncsearch/SubmitAsyncSearchRequest.java

+    /**
+     * Get the {@link SearchRequest} that this submit request wraps
+     */
+    public SearchRequest getSearchRequest() {


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?

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

package protected ? ;)

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

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.

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.

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.

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.

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

...-high-level/src/main/java/org/elasticsearch/client/asyncsearch/SubmitAsyncSearchRequest.java

javanna · 2020-03-18T10:29:12Z

client/rest-high-level/src/test/java/org/elasticsearch/client/asyncsearch/AsyncSearchIT.java

+        SearchSourceBuilder sourceBuilder = new SearchSourceBuilder().query(QueryBuilders.matchAllQuery());
+        SubmitAsyncSearchRequest request = new SubmitAsyncSearchRequest(sourceBuilder, index);
+        // 2 sec should be enough to make sure we always complete right away
+        request.setWaitForCompletion(new TimeValue(2, TimeUnit.SECONDS));


this should work but it's also risky. What other options we have?

increase the timeout even more to really make sure it's enough, though what is enough

accept that we may get a response while the search is still running, in which case is_partial and is_final will have a different value? Maybe don't set cleanOnCompletion and assert that when the id is returned the search is running, and when the id is not returned, both flags are false.

I am not sure either way, maybe I would go for option 1.

We can also have a loop here when get is implemented that submits the initial request and use get until isRunning is false on the response ?

I'll go with 1.) then, staying on the save side. I will keep cleanOnCompletion though, currently thats the only way I can see to actually get an Id and I'd like to tests its deserialization etc... here if possible (will also need ids in a follow-up PR around get-API)

sorry, missed the previous comment. Loop sounds okay as well, getting a response thats not running any more isn't actually the problem, the oposite is tricky (getting a response with an Id, as I mentioned earlier). I don't need that here so will remove it but will need it later for get/delete API tests.

Just also realize I haven't implemented get yet in this PR, we could change this later to a loop with get if you agree?
I don't see the benefits over a long-enough wait time though, quite the oposite. I never got this call to not finish in the first call, neither locally nor on CI which is even faster.

I like the loop idea if it does not make things too complicated, it sounds doable.

it's fine to make this a loop later ;)

cbuescher · 2020-03-18T14:34:24Z

@javanna thanks for the review, I pushed an update that should adress your comments, including hiding the wrapped inner SearchRequest. For that I unfortunately had to add a bunch of additional getters for all the affected parameters that need to be translated in the AsyncRequestConverter and tests. Let me know if you think its worth it.

javanna

left a few more comments. I like it. Only a couple of nits around the changes made to hide the search request

...-high-level/src/main/java/org/elasticsearch/client/asyncsearch/SubmitAsyncSearchRequest.java

client/rest-high-level/src/main/java/org/elasticsearch/client/AsyncSearchRequestConverters.java

…aking getSearchRequest() package private

Currently we don't send values for the `pre_filter_shard_size` and `max_concurrent_shard_requests` SearchRequest parameters over http when using the High Level Rest Client. This change adds these parameters to the RequestConverters and tests.

…est by making getSearchRequest() package private" This reverts commit f686a50.

javanna

LGTM

client/rest-high-level/src/main/java/org/elasticsearch/client/AsyncSearchClient.java

client/rest-high-level/src/main/java/org/elasticsearch/client/AsyncSearchRequestConverters.java

cbuescher · 2020-03-19T10:03:19Z

Thanks @javanna, I pushed updates around your latest comments. @jimczi do you want to take another look or does this look okay now and I can start trying to get this through CI finally?

jimczi

I left some nit comments regarding the async search response parsing but the code looks good to me.

client/rest-high-level/src/main/java/org/elasticsearch/client/RequestConverters.java

.../rest-high-level/src/main/java/org/elasticsearch/client/asyncsearch/AsyncSearchResponse.java

jimczi

LGTM, thanks @cbuescher !

This commit adds a new AsyncSearchClient to the High Level Rest Client which initially supporst the submitAsyncSearch in its blocking and non-blocking flavour. Also adding client side request and response objects and parsing code to parse the xContent output of the client side AsyncSearchResponse together with parsing roundtrip tests and a simple roundtrip integration test. Relates to elastic#49091 Backport of elastic#53592

This commit adds a new AsyncSearchClient to the High Level Rest Client which initially supporst the submitAsyncSearch in its blocking and non-blocking flavour. Also adding client side request and response objects and parsing code to parse the xContent output of the client side AsyncSearchResponse together with parsing roundtrip tests and a simple roundtrip integration test. Relates to #49091 Backport of #53592

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 16, 2020

cbuescher commented Mar 16, 2020

View reviewed changes

client/rest-high-level/src/test/java/org/elasticsearch/client/asyncsearch/AsyncSearchIT.java Outdated Show resolved Hide resolved

iter

ebf5428

jimczi reviewed Mar 16, 2020

View reviewed changes

Christoph Büscher added 4 commits March 16, 2020 18:08

Change supertype of SubmitAsyncSearchRequest

0ed46e6

Adding javadoc

701793e

Overwrite certain SearchRequest paramters on submit request

089485b

Change SearchResponse parsing

87a4f92

jimczi reviewed Mar 17, 2020

View reviewed changes

Christoph Büscher added 4 commits March 17, 2020 10:28

Merge branch 'master' into async-search-hlrc

207441d

Rename response class

8f708af

Don't set CcsMinimizeRountrips in test

b7d6f14

Use setCleanOnCompletion(false) in AsyncSearchIT to always get an ID …

68baeb9

…back

Change ctor of SubmitAsyncSearchRequest

94083b6

jimczi reviewed Mar 17, 2020

View reviewed changes

...-high-level/src/main/java/org/elasticsearch/client/asyncsearch/SubmitAsyncSearchRequest.java Show resolved Hide resolved

Christoph Büscher added 4 commits March 17, 2020 16:24

Merge branch 'master' into async-search-hlrc

0796651

Add request validation and tests

310fdee

iter on failing test

7e02579

Expose batchedReduceSize and batchedReduceSize on submit request

c88625a

javanna reviewed Mar 18, 2020

View reviewed changes

Adressing review comments

3e5a33f

javanna reviewed Mar 18, 2020

View reviewed changes

Christoph Büscher added 6 commits March 18, 2020 16:55

Remove redundant parameters and validation

594a5eb

Avoid additional boilerplate getters on SubmitAsyncSearchRequest by m…

f686a50

…aking getSearchRequest() package private

Revert "Avoid additional boilerplate getters on SubmitAsyncSearchRequ…

4ab7dc7

…est by making getSearchRequest() package private" This reverts commit f686a50.

Add support for max_concurrent_shard_requests

720c966

Merge branch 'master' into async-search-hlrc

220e6a8

javanna approved these changes Mar 18, 2020

View reviewed changes

client/rest-high-level/src/main/java/org/elasticsearch/client/AsyncSearchClient.java Outdated Show resolved Hide resolved

client/rest-high-level/src/main/java/org/elasticsearch/client/AsyncSearchRequestConverters.java Outdated Show resolved Hide resolved

Christoph Büscher added 3 commits March 19, 2020 10:08

Merge branch 'master' into async-search-hlrc

1d0b59c

Add docs urls

ebd1906

Unify conversion of some request parameters

f65cf82

jimczi approved these changes Mar 19, 2020

View reviewed changes

adress review comments

841430e

jimczi approved these changes Mar 19, 2020

View reviewed changes

Christoph Büscher added 2 commits March 19, 2020 15:46

Merge branch 'master' into async-search-hlrc

7d3a8ed

iter

8223989

cbuescher merged commit dea6b7d into elastic:master Mar 19, 2020

cbuescher added the backport pending label Mar 19, 2020

cbuescher mentioned this pull request Mar 20, 2020

Add async_search.submit to HLRC #53592 #53852

Merged

cbuescher removed the backport pending label Mar 20, 2020

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket elastic/elasticsearch-net#4525

Closed

38 tasks

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add async_search.submit to HLRC #53592

Add async_search.submit to HLRC #53592

cbuescher commented Mar 16, 2020

elasticmachine commented Mar 16, 2020

elasticmachine commented Mar 16, 2020

jimczi left a comment

cbuescher commented Mar 17, 2020

cbuescher commented Mar 17, 2020

jimczi left a comment

javanna left a comment

javanna Mar 18, 2020

jimczi Mar 18, 2020

cbuescher Mar 18, 2020

jimczi Mar 18, 2020

cbuescher Mar 18, 2020 •

edited

Loading

cbuescher Mar 18, 2020 •

edited

Loading

cbuescher Mar 18, 2020

javanna Mar 18, 2020

jimczi Mar 18, 2020

cbuescher Mar 18, 2020

javanna Mar 18, 2020

jimczi Mar 18, 2020

cbuescher Mar 18, 2020

cbuescher Mar 18, 2020

cbuescher Mar 18, 2020

javanna Mar 18, 2020

javanna Mar 18, 2020

cbuescher commented Mar 18, 2020

javanna left a comment

javanna left a comment

cbuescher commented Mar 19, 2020

jimczi left a comment

jimczi left a comment

Add async_search.submit to HLRC #53592

Add async_search.submit to HLRC #53592

Conversation

cbuescher commented Mar 16, 2020

elasticmachine commented Mar 16, 2020

elasticmachine commented Mar 16, 2020

jimczi left a comment

Choose a reason for hiding this comment

cbuescher commented Mar 17, 2020

cbuescher commented Mar 17, 2020

jimczi left a comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher Mar 18, 2020 • edited Loading

Choose a reason for hiding this comment

cbuescher Mar 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher commented Mar 18, 2020

javanna left a comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

cbuescher commented Mar 19, 2020

jimczi left a comment

Choose a reason for hiding this comment

jimczi left a comment

Choose a reason for hiding this comment

cbuescher Mar 18, 2020 •

edited

Loading

cbuescher Mar 18, 2020 •

edited

Loading