Skip to content

Remove suggest transport action #17198

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

Merged
merged 10 commits into from
Mar 23, 2016

Conversation

areek
Copy link
Contributor

@areek areek commented Mar 18, 2016

Currently suggest through _search & _suggest endpoints use different transport actions.
The _suggest endpoint uses its own transport action to avoid executing search & aggregration
phases. This change removes dedicated suggest transport action and adds optimization to
shortcut to suggest phase when _search endpoint is used exclusively for suggestions.

This implies:

GET /_suggest
{....}

is actually sugar for:

GET /_search
{
  "suggest" : ....
}

Besides reducing code and complexity of dealing with multiple execution paths for suggest,
this change has two major benefits:

  • simplifies implementing document fetching for completion
  • allows using the search context to process suggestions
    • we can use post_filter / query for filtering suggestions in completion
    • completion can respect _type (through contexts)
    • simplify phrase suggester parameters

API Changes:

This change removes suggest action from the client API:

client().prepareSuggest().addSuggestion("foo", ...)

is equivalent to:

client().prepareSearch().suggest(new SuggestBuilder().addSuggestion("foo", ...))

Behaviour changes:
now we use the search threadpool instead of suggest for suggest-only requests
(would appreciate some feedback there) and the merged suggest stats are representative
of all suggest-only requests either from _suggest or _search endpoints

relates to #10217
simplifies #13576

@s1monw
Copy link
Contributor

s1monw commented Mar 21, 2016

+100 for the diff stats :)

@s1monw
Copy link
Contributor

s1monw commented Mar 21, 2016

@areek this only breaks the java API right?

@areek
Copy link
Contributor Author

areek commented Mar 21, 2016

@s1monw yes, and the suggest enum for the search stats request is a no-op as the suggest stats are merged with the search stats.

@s1monw
Copy link
Contributor

s1monw commented Mar 22, 2016

@s1monw yes, and the suggest enum for the search stats request is a no-op as the suggest stats are merged with the search stats.

I was more curious about the REST endpoints but I guess I will see in a second when I am reviewing :)

/**
* @return true if the request only has suggest
*/
public boolean hasOnlySuggest() {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we name this isSuggestOnly()?

@s1monw
Copy link
Contributor

s1monw commented Mar 22, 2016

I left some minor comments - this looks fantastic! Please add the stats change to the breaking docs too.

thanks so much for this cleanup

@areek areek force-pushed the enhancement/nuke_suggest_transport branch from 08095e8 to 92e5f6f Compare March 23, 2016 16:27
@areek
Copy link
Contributor Author

areek commented Mar 23, 2016

Thanks @s1monw for the review! I added breaking docs for the java api and merging suggest stats into search stats.
we don't use the suggest threadpool at all now (was used only by the dedicated suggest transport action). any thoughts? if we are ok with using search threadpool, we should remove the suggest threadpool all together.

@s1monw
Copy link
Contributor

s1monw commented Mar 23, 2016

+1 on removing suggest threadpool

@s1monw
Copy link
Contributor

s1monw commented Mar 23, 2016

LGTM - awesome lets remove the threadpool in a sep PR, we also need a breaking doc addition for that

@areek areek force-pushed the enhancement/nuke_suggest_transport branch from d32c1f4 to f0a182e Compare March 23, 2016 19:34
@areek areek force-pushed the enhancement/nuke_suggest_transport branch from f0a182e to 442a6e0 Compare March 23, 2016 20:38
@areek areek merged commit 442a6e0 into elastic:master Mar 23, 2016
@areek
Copy link
Contributor Author

areek commented Mar 23, 2016

Thanks @s1monw for the review! I will open a PR for removing suggest threadpool

areek added a commit to areek/elasticsearch that referenced this pull request Mar 23, 2016
In elastic#17198, we removed suggest transport action, which
used the `suggest` threadpool to execute requests. Now
`suggest` threadpool is unused and suggest requests are
executed on the `search` threadpool.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking-java :Search Relevance/Suggesters "Did you mean" and suggestions as you type v5.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants