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

Add POST /_search/clear_scroll endpoint and deprecate delete scroll endpoint #21510

Closed
wants to merge 3 commits into from

Conversation

javanna
Copy link
Member

@javanna javanna commented Nov 11, 2016

The clear scroll api currently allows to provide a scroll by specifying it either as part of the url (it is effectively the resource that gets deleted) or within the request body. The current api uses the DELETE method though, and we have decided to remove support for providing the request body with any DELETE endpoint in the future. In order to get to this for the next major version, we introduce the new endpoint POST /_search/clear_scroll which replaces the current clear_scroll api and uses POST instead of DELETE. It allows to provide the scroll_id as a url parameter, which is though deprecated (will output a deprecation warning when used) in favour of providing it as part of the request body.

The DELETE /_search/scroll/ is deprecated, hence it will output a deprecation warning whenever used. The DELETE endpoints will be removed in 6.0, as well as the support for providing the scroll_id as a url parameter against the POST endpoint.

Relates to #8217
Relates to #21453

@@ -62,7 +62,7 @@
- length: {hits.hits: 0 }

- do:
clear_scroll:
delete_scroll:
Copy link
Member Author

Choose a reason for hiding this comment

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

this is pretty much the breaking change in terms of api. the new clear_scroll doesn't support the scroll_id parameter, so these calls need to be migrated to delete_scroll, or the scroll needs to be provided as part of the request body. Not sure what kinds of problems this can cause in the clients, maybe somebody has suggestions

@polyfractal
Copy link
Contributor

Technically, this break will work fine for the clients in regards to testing. E.g. if the client doesn't support the new delete_scroll, it'll just fail the test (as it should).

But for the end-user, this is gonna be problematic. If an end-user is currently using clear_scroll, they provide the scroll ID as part of the endpoint params, e.g:

$client->clear_scroll([
  'scrollId' => 'abc123'
]);

But this change will make that illegal, so they have to adjust their code to move it into the body:

$client->clear_scroll([
  'body' => [
    'scrollId' => 'abc123'
  ]
]);

Or change code to use the new delete_scroll() endpoint. Either way, it's a code-change in a minor release.

So any of the clients following SemVer are going to be in a sticky place, since this is a pretty noticeable API breaking change for a 5.x release. They'd really have to bump the major version to allow this, or start doing magic under the covers to insert the scroll ID into the body (which isn't currently done anywhere, to my knowledge).

To summarize:

  • clear_scroll -> delete_scroll is fine
  • Adding back a POST clear_scroll is problematic, since client's can't leave the old DELETE clear_scroll laying around as a deprecated (but usable) endpoint. Ideally, the new POST version would be named something entirely different so it's just adding functionality, not breaking.

@javanna
Copy link
Member Author

javanna commented Nov 14, 2016

thanks a lot for your explanation @polyfractal ! I have no intentions to make a breaking change for the clients with this PR, I was just not sure it would break anything. The rename was ideal, as calling the new endpoint delete sounded funny because it uses POST. At this point I would leave clear_scroll as-is with the delete method and come up with a different name for the POST endpoint. Makes sense?

@polyfractal
Copy link
Contributor

++ that'd work. :)

I'm afraid I don't have any ideas about an alternative name though :(

@javanna javanna force-pushed the enhancement/clear_scroll_post branch 2 times, most recently from 7b12249 to b6e8c4b Compare November 21, 2016 15:20
@javanna
Copy link
Member Author

javanna commented Nov 21, 2016

I made the change fully bw compatible now, @polyfractal mind having another look? Maybe @clintongormley wants to look too?

@@ -0,0 +1,16 @@
{
"clear_scroll_post": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this should be post_clear_scroll, just to follow the http-verb-preceding-noun convention (put_template, etc)? TBH my level of caring is like 10% though :) @elastic/es-clients any thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense to me

@polyfractal
Copy link
Contributor

Left a minor comment, and I don't particularly care which way it goes.

REST stuff LGTM! Thanks for making this client friendly :)

@clintongormley
Copy link

What about doing the following instead:

  • POST _search/clear_scroll accepts a body with the scroll ID
  • POST _search/clear_scroll also accepts the URL and query string param form of scroll_id (but add them as deprecated)
  • change the clear_scroll REST spec to use POST _search/clear_scroll
  • deprecate use of DELETE _search/scroll

This way the clients will continue to work as before, and users will get the deprecated warnings if they try to pass the scroll ID in the URL/query string.

@javanna
Copy link
Member Author

javanna commented Nov 22, 2016

@clintongormley did you mean to deprecate DELETE _search/scroll completely? That would mean that the only correct way to provide a scroll_id would be via POST and by providing it as part of the request body? I thought that we wanted to leave the DELETE _search/scroll/${scroll_id} endpoint around. Also having a new clean api that only accepts the body for POST, that could be a better trade-off than having deprecation warnings in there too? What do you think?

@clintongormley
Copy link

did you mean to deprecate DELETE _search/scroll completely?

Yes. Why have two ways to do it?

That would mean that the only correct way to provide a scroll_id would be via POST and by providing it as part of the request body?

Yes, it's the only reliable way to do it. For the URL and QS versions, it fails when your scroll IDs are too long.

Also having a new clean api that only accepts the body for POST, that could be a better trade-off than having deprecation warnings in there too? What do you think?

This is to handle the change in the client. Users will keep using the clear_scroll method, but internally it'll use the POST endpoint. But they may well be calling it as clear_scroll( scroll_id => 'foo') when they should move to passing it in the body.

So we don't break anything for the user in 5.x, but instead warn them that this will break in 6.0

…ndpoint

The clear scroll api currently allows to provide a scroll by specifying it either as part of the url (it is effectively the resource that gets deleted) or within the request body. The current api uses the DELETE method though, and we have decided to remove support for providing the request body with any DELETE endpoint in the future. In order to get to this for the next major version, we introduce the  new endpoint `POST /_search/clear_scroll` which replaces the current clear_scroll api and uses POST instead of DELETE. It allows to provide the `scroll_id` as a url parameter, which is though deprecated (will output a deprecation warning when used) in favour of providing it as part of the request body.

 The `DELETE /_search/scroll/` is deprecated, hence it will output a deprecation warning whenever used. The DELETE endpoints will be removed in 6.0, as well as the support for providing the scroll_id as a url parameter against the POST endpoint.

Relates to elastic#8217
Relates to elastic#21453
@javanna javanna force-pushed the enhancement/clear_scroll_post branch from b6e8c4b to 50403ed Compare November 23, 2016 10:49
@javanna javanna changed the title Add POST /_search/clear_scroll endpoint and deprecate delete scroll with request body Add POST /_search/clear_scroll endpoint and deprecate delete scroll endpoint Nov 23, 2016
@javanna javanna added v5.2.0 and removed v5.1.1 labels Nov 23, 2016
@javanna
Copy link
Member Author

javanna commented Nov 23, 2016

thanks @clintongormley I have updated content, title and description of the PR.

{
"scroll_id" : [
"_all"
]
Copy link
Member Author

Choose a reason for hiding this comment

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

this one looks a bit odd, looked better providing _all as part of the url, but maybe that's ok

Choose a reason for hiding this comment

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

Perhaps this could be replaced with supporting * as a scroll ID

Choose a reason for hiding this comment

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

Actually we probably don't support wildcard matching, so using _all is better

}
if (request.method() == POST && request.hasParam("scroll_id")) {
deprecationLogger.deprecated("Deprecated [POST /_search/clear_scroll/{scroll_id}] endpoint used, " +
"expected [POST /_search/clear_scroll] instead. The scroll_id should rather be provided as part of the request body");

Choose a reason for hiding this comment

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

should rather -> must

@@ -165,23 +165,24 @@ DELETE /_search/scroll
// CONSOLE
// TEST[catch:missing]

All search contexts can be cleared with the `_all` parameter:
The recommended way to provide the `scroll_id` is by placing it in the request

Choose a reason for hiding this comment

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

Possibly replace this block with:

deprecated[5.2.0,Providing the scroll ID as part of the URL or as a query string parameter has been deprecated in favour of specifying it in the request body.  The `DELETE _search/scroll` end points are deprecated in favour of `POST _search/clear_scroll]

@@ -14,7 +14,7 @@
"params": {}
},
"body": {
"description": "A comma-separated list of scroll IDs to clear if none was specified via the scroll_id parameter"
"description": "A comma-separated list of scroll IDs to clear, has the precedence over the scroll_id parameter"

Choose a reason for hiding this comment

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

has precedence

@@ -0,0 +1,20 @@
{

Choose a reason for hiding this comment

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

Why add this at all? This will result in adding a new method to the clients.

Choose a reason for hiding this comment

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

Is it just there for bwc testing?

Copy link
Member Author

@javanna javanna Nov 23, 2016

Choose a reason for hiding this comment

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

no you are right. it is a leftover, due to the fact that I had thought POST shouldn't support the query_string parameter at all, now that it does, I can get right of this new api.

Copy link
Member Author

@javanna javanna Nov 23, 2016

Choose a reason for hiding this comment

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

oh wait though, DELETE is done against _search/scroll while POST is done against _search/clear_scroll . I don't think we can express that in our spec without having two apis. POST _search/scroll is already taken by the search api.

@javanna
Copy link
Member Author

javanna commented Jan 30, 2017

quick update: this depends on #21888 otherwise it becomes very hard to properly test the change.

@javanna
Copy link
Member Author

javanna commented Aug 3, 2017

why was this closed @martijnvg ?

@javanna javanna changed the base branch from 5.x to 5.6 August 3, 2017 06:57
@javanna javanna reopened this Aug 3, 2017
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Scroll labels Feb 14, 2018
@cbuescher
Copy link
Member

@javanna is this PR still something we want to do?
//cc @elastic/es-search-aggs

@javanna
Copy link
Member Author

javanna commented Mar 13, 2018

yes

@colings86
Copy link
Contributor

@javanna since #21888 is now resolved is this able to progress now (should we remove the stalled label)?

@javanna
Copy link
Member Author

javanna commented Oct 24, 2018

yes I was just thinking about this recently. I guess I will need to open a new PR at this point, I need to find time to get back to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>deprecation >enhancement :Search/Search Search-related issues that do not fall into other categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants