openservicebrokerapi / servicebroker

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

Make 408's not result in orphan mitigation #456

Closed duglin closed 6 years ago

duglin commented 6 years ago

/cc @nilebox

Closes #449

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

cfdreddbot commented 6 years ago

Hey duglin!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

duglin commented 6 years ago

While we could technically merge the two 4xx lines, I think its important to call out the 408 by itself so we can be clear that this is a "client side" timeout and not a "server side" one - which are very very different w.r.t. orphan mitigation

nilebox commented 6 years ago

TBH I would prefer to remove the special mention of 408 completely, as it's not actually special as we discussed in #449. All 4xx are client side errors, and there are no exceptions in regards to orphan mitigation. We also don't mention the 504 Gateway Timeout explicitly anywhere apart from 5xx, for example.

duglin commented 6 years ago

@nilebox my original version of the PR did merge them and within 5 minutes I got a ping about it because the distinction between client-side vs server-side timeout wasn't clear - so I added it back in, just for the sake of clarity. We don't need to call-out 504 because it results in orphan mitigation which is what most people would expect from a "timeout".

nilebox commented 6 years ago

@duglin I see your concern, but the distinction between client-side and server-side errors is an HTTP standard's problem, OSB doesn't bring anything additional there. Furthemore, HTTP standard makes the distinction very clear - 4xx are client-side errors, and 5xx are server-side errors. If we needed to alter this distinction (which OSB spec currently does), it would obviously require a special wording in the OSB spec. But since we don't (anymore), I don't see the value of keeping 408 separate from 4xx.

n3wscott commented 6 years ago

LGTM

Approved with PullApprove

duglin commented 6 years ago

reviews needed

fmui commented 6 years ago

LGTM

Approved with PullApprove

mattmcneeney commented 6 years ago

LGTM

Approved with PullApprove

zrob commented 6 years ago

lgtm

Approved with PullApprove

n3wscott commented 6 years ago

LGTM

Approved with PullApprove

duglin commented 6 years ago

2 more reviews needed @fmui @vaikas-google @mattmcneeney

fmui commented 6 years ago

LGTM

Approved with PullApprove

vaikas commented 6 years ago

LGTM

Approved with PullApprove