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

Deprecate _suggest endpoint in favour of _search #20305

Merged
merged 7 commits into from Dec 15, 2016

Conversation

Projects
None yet
4 participants
@areek
Copy link
Contributor

commented Sep 2, 2016

In 5.0, _search endpoint has been optimized for suggest
only search requests. Users should move away from
using the _suggest endpoint, as it may be deprecated and
removed in the future.

@clintongormley

This comment has been minimized.

Copy link
Member

commented Sep 7, 2016

I don't think we should suggest not using _suggest because it might be deprecated. We should either deprecate it or not. I'd be good with deprecating.

@Mpdreamz

This comment has been minimized.

Copy link
Member

commented Sep 12, 2016

+1 for deprecation, the dedicated endpoint returns only a marginally smaller response.
If _search and _suggest now have same performance characteristics when only doing suggestions _suggest now feels superfluous for the same reason we have no dedicated /_aggs endpoint 👍

@nik9000

View changes

docs/reference/search/suggesters.asciidoc Outdated
"my-suggest-2": ...
"hits": ...
"took": ...
"timed_out": ...

This comment has been minimized.

Copy link
@nik9000

nik9000 Sep 12, 2016

Contributor

I'd just say "timed_out": false, and "took": <whatever number it take on the first run on your laptop>,. I like ... for when you are eliding large structures but for short stuff like this I'd just let it be an example. You still need the s/// clause for took but I think it makes the docs easier to read if the reader doesn't have to guess what expected values for took are.

You could also do something like:

  ...,
  "suggest": {
  }

To elide the whole search response. I don't think that is a particularly good idea here because it hides that this is a search response. But it is a valid strategy. You could do it with // TESTRESPONSE [s/...,/"hits": "$body.hits", "took": "$body.took", "timed_out": "$body.timed_out"/].

@areek

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2016

I'd be good with deprecating.

++, I opened #20435 deprecating _suggest endpoint.

Replace _suggest endpoint to _search in docs
In 5.0, the _suggest endpoint is just sugar for _search
with suggestions specified. Users should move away from
using the _suggest endpoint, as it may be deprecated and
removed in the future

@areek areek force-pushed the areek:docs/fix_suggest_endpoint branch to b47692a Sep 13, 2016

@areek

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2016

@nik9000 Can you take look? In this PR:

  • updated all docs and rest-tests to use _search instead of _suggest endpoint
  • added deprecation logging to _suggest endpoint
  • added a rest test to ensure deprecation warning is set when _suggest is used

After this gets in, I will backport the change to 5.x and then remove _suggest endpoint entirely in master as planned in #20435 (comment)

@areek areek changed the title Replace _suggest endpoint with _search in docs Deprecate _suggest endpoint in favour of _search Dec 14, 2016

@areek areek force-pushed the areek:docs/fix_suggest_endpoint branch Dec 14, 2016

@nik9000
Copy link
Contributor

left a comment

Left some minor stuff, otherwise LGTM.

@@ -1,7 +1,7 @@
{
"suggest": {
"documentation": "http://www.elastic.co/guide/en/elasticsearch/reference/master/search-suggesters.html",
"methods": ["POST", "GET"],
"methods": ["POST"],

This comment has been minimized.

Copy link
@nik9000

nik9000 Dec 14, 2016

Contributor

It looks like GET is still supported. Maybe revert all changes to this file?

This comment has been minimized.

Copy link
@areek

areek Dec 14, 2016

Author Contributor

I removed the GET here to make sure we can test the deprecation warning here. Unfortunately, afaik the warnings message has to be an exact match.

This comment has been minimized.

Copy link
@nik9000

nik9000 Dec 14, 2016

Contributor

Ah! That makes sense. I believe it does have to match exactly.

This comment has been minimized.

Copy link
@nik9000

nik9000 Dec 14, 2016

Contributor

Then this is fine with me.

@@ -50,15 +50,16 @@ setup:
indices.refresh: {}

- do:
suggest:

This comment has been minimized.

Copy link
@nik9000

nik9000 Dec 14, 2016

Contributor

Maybe it makes sense to move these to test/search/110_completion.yaml, etc.

This comment has been minimized.

Copy link
@areek

areek Dec 14, 2016

Author Contributor

thanks for the suggestion, on it

@areek areek force-pushed the areek:docs/fix_suggest_endpoint branch to d0c53ff Dec 14, 2016

@areek areek merged commit cdd5fbe into elastic:master Dec 15, 2016

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details

areek added a commit that referenced this pull request Dec 15, 2016

Deprecate _suggest endpoint in favour of _search (#20305)
* Replace _suggest endpoint to _search in docs

In 5.0, the _suggest endpoint is just sugar for _search
with suggestions specified. Users should move away from
using the _suggest endpoint, as it is marked as deprecated in 5.x and
will be removed in 6.0

* update docs to use _search endpoint instead of _suggest

* Add deprecation logging to RestSuggestAction

* Use search endpoint instead of suggest endpoint in rest tests
@areek

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2016

backported to 5.x

areek added a commit to areek/elasticsearch that referenced this pull request Dec 15, 2016

Remove deprecated _suggest endpoint
In elastic#20305, _suggest endpoint was deprecated
in favour of using _search endpoint. This
commit removes the dedicated _suggest endpoint
entirely from master.

areek added a commit that referenced this pull request Dec 16, 2016

Remove deprecated _suggest endpoint (#22203)
In #20305, _suggest endpoint was deprecated
in favour of using _search endpoint. This
commit removes the dedicated _suggest endpoint
entirely from master.

Mpdreamz added a commit to elastic/elasticsearch-net that referenced this pull request Jan 30, 2017

Mpdreamz added a commit to elastic/elasticsearch-net that referenced this pull request Feb 2, 2017

Mpdreamz added a commit to elastic/elasticsearch-net that referenced this pull request Feb 6, 2017

Mpdreamz added a commit to elastic/elasticsearch-net that referenced this pull request Feb 6, 2017

Mpdreamz added a commit to elastic/elasticsearch-net that referenced this pull request Feb 6, 2017

awelburn added a commit to Artesian/elasticsearch-net that referenced this pull request Nov 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.