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 Support for Deprecated REST Handler #17687

Closed
pickypg opened this issue Apr 12, 2016 · 6 comments
Closed

Add Support for Deprecated REST Handler #17687

pickypg opened this issue Apr 12, 2016 · 6 comments
Assignees
Labels
:Core/Infra/Logging Log management and logging utilities :Core/Infra/REST API REST infrastructure and utilities v5.0.0-alpha5

Comments

@pickypg
Copy link
Member

pickypg commented Apr 12, 2016

As we change our REST Endpoints, particularly with simple examples like going from _optimize to _forcemerge, we should use the deprecation logger to announce the deprecation to the user.

Hopefully it's as simple as adding something along the lines of

controller.registerDeprecatedHandler(POST, "/_optimize", this);
controller.registerDeprecatedHandler(POST, "/{index}/_optimize", this);

I'm interested to hear what people might want to see as part of the deprecation notice though. I wouldn't want to make it too fancy, but it might be nice to say something along the lines of "see _forcemerge".

What do you think @clintongormley?

@pickypg pickypg added discuss :Core/Infra/Logging Log management and logging utilities :Core/Infra/REST API REST infrastructure and utilities v5.0.0-alpha2 labels Apr 12, 2016
@uboness
Copy link
Contributor

uboness commented Apr 12, 2016

align the message with the deprecated settings message?

"Endpoint [xxxxxx] is deprecated, use [yyyyyy] instead"

do we also want:

controller.registerRemovedEndpoint(POST, "/_optimize");

that will emit:

"Endpoint [xxxxxx] has been removed, use [yyyyyy] instead"

@jasontedor
Copy link
Member

As a bonanza, adding an HTTP header containing the same might be useful (the client might not have access to the server logs where the deprecation logging would be emitted)?

@clintongormley
Copy link
Contributor

++

As a bonanza, adding an HTTP header containing the same might be useful (the client might not have access to the server logs where the deprecation logging would be emitted)?

I was going to comment that the clients don't expose headers at all, so this info would be hidden. But actually, the clients all have logging facilities. I think the clients should look for these headers and warn if found. This would improve the discoverability of deprecations (#8963) which currently require deprecation logging to be specifically enabled. It'd also make it easier to figure out which requests are using deprecated syntax.

@pickypg
Copy link
Member Author

pickypg commented Apr 13, 2016

Is there a preferred, existing header for this?

As a side note, I took a look for a better response code, and there does exist 299 as a warn code in the 2xx line of response codes. The other 2xx response codes seem more exact in nature.

Along with 299, it also brings along some other headers that seem highly relevant: https://tools.ietf.org/html/rfc7234#section-5.5

The "Warning" header field is used to carry additional information
about the status or transformation of a message that might not be
reflected in the status code. This information is typically used to
warn about possible incorrectness introduced by caching operations or
transformations applied to the payload of the message.

Warnings can be used for other purposes, both cache-related and
otherwise. The use of a warning, rather than an error status code,
distinguishes these responses from true failures.

This would allow us to signal a valid response without breaking anyone that checks for a [200, 300) response code and provide it in a standardized format. What do you think?

I think that the way it should be handled is:

if response code is 200 or 201, then
   change to 299
end

add warning header info

What do we think?

@jasontedor
Copy link
Member

Is there a preferred, existing header for this?

I think that we can use the Warning: header.

I do not think that we should change the status code. I especially do not think that we should use 299 from RFC 7234; while RFC 7234 is published on the Standards Track it is still in Proposed Standard status (as of 2016-04-13). I doubt that clients will properly handle this.

@clintongormley clintongormley added help wanted adoptme and removed discuss labels Apr 14, 2016
@clintongormley
Copy link
Contributor

@elastic/es-clients see #17687 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Logging Log management and logging utilities :Core/Infra/REST API REST infrastructure and utilities v5.0.0-alpha5
Projects
None yet
Development

No branches or pull requests

4 participants