openservicebrokerapi / servicebroker

Open Service Broker API Specification
https://openservicebrokerapi.org/
Apache License 2.0
1.19k stars 436 forks source link

Add a delay to polling response #621

Closed duglin closed 5 years ago

duglin commented 5 years ago

First part of #606

Adds a poll_delay field to the last_operation response to allow a Broker to tell a Platform to back-off on its polling

Signed-off-by: Doug Davis dug@us.ibm.com

cfdreddbot commented 5 years ago

:white_check_mark: Hey duglin! The commit authors and yourself have already signed the CLA.

tinygrasshopper commented 5 years ago

I think we should leverage the HTTP caching mechanism, to optimize the polling. If the broker sets Cache-Control: max-age= header on the last operation resource, it would convey to the platform that this resource is not needed to be GETted again for the specified time. That would make the platform implementation simpler as well by leveraging a HTTP client extension which can respect the cache control header.

duglin commented 5 years ago

I'm not sure how I feel about using the http header. The http spec says The expiration mechanism applies only to responses taken from a cache and not to first-hand responses forwarded immediately to the requesting client., which isn't quite what we're doing. What do other people think?

mattmcneeney commented 5 years ago

What are we going to do about this one @duglin ?

duglin commented 5 years ago

On a previous call I believe it was @fmui who pointed out that the Retry-After header exists. The docs for that header (https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html) only talk about it being used for 503's and 3xx's, and we'd want to use it on a 200. So I did some digging and found this: https://tools.ietf.org/html/rfc6585 which talks about it being used on a 429. This addresses something I was wondering.... can it be used for non 503's and 3xx's ? The original spec doesn't say it can't - and RFC6585 proves that other people are interpreting it the same way. :-)

I've modified this PR to use that header instead. What do other people think?

mattmcneeney commented 5 years ago

@duglin I think you may need to update openapi.yaml too

mattmcneeney commented 5 years ago

I think I'm happy with this proposal, but I just wanted to highlight this text (from here):

The Retry-After response HTTP header indicates how long the user agent should wait before making a follow-up request. There are three main cases this header is used:

- When sent with a 503 (Service Unavailable) response, this indicates how long the service is expected to be unavailable.
- When sent with a 429 (Too Many Requests) response, this indicates how long to wait before making a new request.
- When sent with a redirect response, such as 301 (Moved Permanently), this indicates the minimum time that the user agent is asked to wait before issuing the redirected request.

It doesn't say that we can't use the header for this situation, but doesn't call out this case either.

duglin commented 5 years ago

@mattmcneeney I updated openapi.yaml now - I also fixed the swagger file, not sure what I was thinking putting it on the request instead of the response!

mattmcneeney commented 5 years ago

Trying to retrigger the Travis CI build now...

mattmcneeney commented 5 years ago

Travis just passed, but doesn't seem to have updated this PR... https://travis-ci.org/openservicebrokerapi/servicebroker/builds/469807918

cfdreddbot commented 5 years ago

:white_check_mark: Hey duglin! The commit authors and yourself have already signed the CLA.

mattmcneeney commented 5 years ago

Closed and re-opened to see if travis would be triggered ...

cfdreddbot commented 5 years ago

:white_check_mark: Hey duglin! The commit authors and yourself have already signed the CLA.

cfdreddbot commented 5 years ago

:white_check_mark: Hey duglin! The commit authors and yourself have already signed the CLA.

mattmcneeney commented 5 years ago

Travis is working again at last! Phew!

georgi-lozev commented 5 years ago

What is supposed to be the value? Milliseconds, seconds, minutes? I presume "an integer number of seconds (in decimal)" based on https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html, but should the spec have an opinion on that?

What should the platform do in case the broker provides date and not duration as per the recommendation? Error or ignore and stick to some default.

CC @laurzca @mattmcneeney @fmui

mattmcneeney commented 5 years ago

My assumption was that Platforms would try to handle both a HTTP-date or delta-seconds as outlined in the spec. But we could be more restrictive in here (e.g. enforce a time interval in seconds) if we want to make it easier for Platforms?