Add search method to high level REST client#24796
Conversation
There was a problem hiding this comment.
we should probably introduce some kind of setting in the client that allows to choose whether we should use POST or GET when there's a body.
tlrx
left a comment
There was a problem hiding this comment.
LGTM, I left some minor comments
There was a problem hiding this comment.
Can we renamed it SEARCH_REQUEST_CONTENT_TYPE?
There was a problem hiding this comment.
it is supposed to be used in all requests that have a body which we generate calling toXContent. It is not just about search (although it is at the moment)
There was a problem hiding this comment.
Then we'll need to add support for scroll search
There was a problem hiding this comment.
Collections.emptySet() is already statically imported so you can use emptySet()
There was a problem hiding this comment.
Could we rename this method to getDefaultNamedXContents()?
There was a problem hiding this comment.
I think we could have a method that provides the map of named aggregations and another for the suggestions.
There was a problem hiding this comment.
Would be nice to use CompletionSuggestion.NAME etc too
There was a problem hiding this comment.
yes, I wasn't sure, I copied this from SuggestTests :) will change
There was a problem hiding this comment.
I thought about this as well, but why do we need to separate the two?
There was a problem hiding this comment.
I think we could check that successful == total shards and that total shards is greater than zero
There was a problem hiding this comment.
because we rely on defaults that may change?
There was a problem hiding this comment.
nit: maybe test with source more often (frequently?)
There was a problem hiding this comment.
yep Tanguy made the same comment
There was a problem hiding this comment.
How do we register the parset for the "children" aggregation here? After all its in its own module.
There was a problem hiding this comment.
Same for the "matrix_stats" aggregation.
There was a problem hiding this comment.
we just don't :) I guess I should add that
There was a problem hiding this comment.
I'll take a look at this - One way is to have a RestHighLevelClient that accepts SearchPlugins and use the getAggregations() getSuggester() specs
There was a problem hiding this comment.
I added support for children and matrix stats. We will ship with their jar. Plugins will be a different story.
There was a problem hiding this comment.
Some if these assertions look quiet similar for most test cases here, maybe they can be moved to a single helper method?
47df9d4 to
c7ca3de
Compare
This will be useful for the high level client to add support for the matrix stats aggregation, as we will ship with this jar by default like we do for parent-join-client which is aligned with distributing core with the modules already included. Relates to elastic#24796
This will be useful for the high level client to add support for the matrix stats aggregation, as we will ship with this jar by default like we do for parent-join-client which is aligned with distributing core with the modules already included. Relates to #24796
This will be useful for the high level client to add support for the matrix stats aggregation, as we will ship with this jar by default like we do for parent-join-client which is aligned with distributing core with the modules already included. Relates to #24796
729bc7a to
6a25920
Compare
|
I addressed comments and added tests for matrix-stats and children aggregation. Can you have another look please? |
* master: Add a second refresh to concurrent relocation test Add a dummy_index to upgrade tests to ensure we recover fine with replicas (elastic#24937) Rework bwc snapshot projects to build up to two bwc versions (elastic#24870) Move the IndexDeletionPolicy to be engine internal (elastic#24930) [Tests] Harden InternalExtendedStatsTests (elastic#24934) TCorrecting api name (elastic#24924) Add search method to high level REST client (elastic#24796) Add fromXContent method to ClearScrollResponse (elastic#24909) ClearScrollRequest to implement ToXContentObject (elastic#24907) SearchScrollRequest to implement ToXContentObject (elastic#24906) Fix bug in weight computation for query cache
This PR is currently based on #24795 and #24794 . It will go in after them. It is also opened against the feature/client_aggs_parsing as it is based on the work done on the branch, but it will go directly to master once the branch is merged.