HLRC: Don't send defaults for SubmitAsyncSearchRequest

[cbuescher]

Currently we set the defaults for ccsMinimizeRoundtrips, preFilterShardSize and
requestCache on the HLRC SubmitAsyncSearchRequest in the constructor. This is no
longer needed since we now only send the parameters along with the rest request
that are supported (omitting e.g. ccsMinimizeRoundtrips) and the correct
defaults are set on the client side. This change removes setting and sending
these defaults where possible, leaving only the overwrite of batchedReduceSize
with a default value of 5, since the default used in the vanilla SearchRequest
is 512. However, we don't need to send this value along as a request parameter
if its the default since the correct one will be set on the receiving end if no
value is specified.
Also adding tests for RestSubmitAsyncSearchAction that check the correct
defaults are set when parameters are missing on the server side.

[elasticmachine]

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

[elasticmachine]

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

[cbuescher]

fyi I just see this needs updates from #54198, will merge that in once its on master...

[bpintea]

7.7 has been features frozen, can the v7.7.0 label be removed?
Please disregard, I'll postpone issues tracking till later in the release process when things get a bit firmer.

[javanna]

LGTM besides the comment I left

[javanna]

I would prefer if we found a way to not have the default value in the client too. Would it be an option to make this an Integer that is null by default, and only set it when it was explicitly set?

[cbuescher]

That would be an Integer in the SubmitAsyncSearchRequest, masking the one in the wrapped SearchRequest, I think. I find that equally confusing from the code side tbh, we would keep most parameters set on the internal search request but only this in the async submit request? With the current defautl its at least clear when looking at the code what value is used when the user doesn't set this? wdyt?

[javanna]

The problem with the current approach is that changing default value which is not do unlikely for this parameter, would require changing both server and client. Also what happens if we change the default value but I've uses an earlier version of the client with the previous default?

…

On Thu, Mar 26, 2020, 11:11 Christoph Büscher ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In client/rest-high-level/src/main/java/org/elasticsearch/client/AsyncSearchRequestConverters.java <#54200 (comment)> : > @@ -73,7 +73,9 @@ static void addSearchRequestParams(Params params, SubmitAsyncSearchRequest reque if (request.getAllowPartialSearchResults() != null) { params.withAllowPartialResults(request.getAllowPartialSearchResults()); } - params.withBatchedReduceSize(request.getBatchedReduceSize()); + if (request.getBatchedReduceSize() != SubmitAsyncSearchRequest.DEFAULT_BATCHED_REDUCE_SIZE) { That would be an Integer in the SubmitAsyncSearchRequest, masking the one in the wrapped SearchRequest, I think. I find that equally confusing from the code side tbh, we would keep most parameters set on the internal search request but only this in the async submit request? With the current defautl its at least clear when looking at the code what value is used when the user doesn't set this? wdyt? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#54200 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGLHTHRZWSLDWGDMMNL4GLRJMS4XANCNFSM4LTRWMOA> .

[cbuescher]

@javanna I see, take a look at e594083, does that look better to you?

[javanna]

heya @cbuescher I definitely see what you mean, but I think the new approach is better. Note that once #51857 is resolved we may even unify the default value with search and then this problem goes away.