Clean up some text around parallel requests #193
Conversation
|
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. |
spec.md
Outdated
| other activity being done then the broker MUST reject the request with | ||
| an HTTP `400 Bad Request` response. The HTTP body of the response MUST | ||
| include a `description` property explaining the reason for the failure. | ||
| The platform SHOULD then retry the request again later. |
There was a problem hiding this comment.
I feel uncomfortable telling platforms what they SHOULD do. Moreover, a later retry is sort of implicit. Is this sentence overkill?
There was a problem hiding this comment.
I don't mind dropping it. I only added it because I didn't want people to think that an error means "stop" - in this case it means "try again later". You never know what's in people's heads. :-)
There was a problem hiding this comment.
No strong feelings, let's see what other people think.
There was a problem hiding this comment.
It may not be obvious to everyone when a request will be rejected (ie. trying to bind whilst an instance is still being provisioned). A broker could always accept an async binding (if supported) whilst a provision is taking place if it wanted to?
There was a problem hiding this comment.
I had trouble with the language in that first sentence. Was trying to find a way to say "be smart about it", but technically you're correct that when a broker will accept a request vs claim its busy is hard to determine. Perhaps instead we should just talk about how some brokers will only be able to process/accept serialized commands for a resource so platforms will need to be prepared, if they send things in parallel, for some brokers to reject secondary requests.
What about:
This specification places no requirement on brokers with respect to their
support for processing concurrent requests that act upon the same set of
resources. If a broker receives a request that it is not able to process due to
other activity being done then the broker MUST reject the request with
an HTTP `400 Bad Request` response. The HTTP body of the response MUST
include a `description` property explaining the reason for the failure.
The platform SHOULD then retry the request again later.
There was a problem hiding this comment.
I think that's good, but maybe the last sentence is redundant?
The platform may decide later on that whatever it wanted to do is no longer useful, so I don't think we need to specify any platform behaviour here. It's more of a "The platform can..." IMO. I like this much more apart from that though.
|
rebased |
|
LGTM |
|
LGTM |
spec.md
Outdated
| @@ -270,7 +270,19 @@ An asynchronous response triggers the platform marketplace to poll the endpoint | |||
|
|
|||
| #### Blocking Operations | |||
|
|
|||
| The marketplace MUST ensure that service brokers do not receive requests for an instance while an asynchronous operation is in progress. For example, if a broker is in the process of provisioning an instance asynchronously, the marketplace can not allow any update, bind, unbind, or deprovision requests to be made through the platform. A user who attempts to perform one of these actions while an operation is already in progress MUST receive a `400 Bad Request` response with the error message: `Another operation for this service instance is in progress`. | |||
| This specification places no requirement on brokers with respect to their | |||
There was a problem hiding this comment.
I don't think the first sentence of this paragraph agrees with the second sentence, but I get the idea of the first. Is the first sentence better put as: "brokers are not required to support parallel requests on the same set of resources". The requirement is actually the opposite of supporting parallel requests - broker's are required to reject parallel requests.
There was a problem hiding this comment.
Right I was trying to say they're not required to support parallel requests and if they don't THIS is what they're supposed to return. I'll use your wording...
spec.md
Outdated
| support for processing concurrent requests that act upon the same set of | ||
| resources. If a broker receives a request that it is not able to process | ||
| due to other activity being done then the broker MUST reject the request | ||
| with an HTTP `400 Bad Request` response. The HTTP body of the response MUST |
There was a problem hiding this comment.
The convention elsewhere in the spec is to return a 422 if a broker can't perform something that some brokers can. Unless we say that broker MUST not support parallel operations, then a 400 doesn't seem appropriate.
|
ok ready for more reviews |
|
LGTM |
1 similar comment
|
LGTM |
Really this section shouldn't be just about asyc stuff, but instead dealing with more than one request at a time. Signed-off-by: Doug Davis <[email protected]>
|
rebased - more L T G Ms needed |
|
LGTM go go go |
|
LGTM |
1 similar comment
|
LGTM |
|
One more @pmorie / @vaikas-google if happy |
|
LGTM |
Really this section shouldn't be just about asyc stuff, but instead
dealing with more than one request at a time.
Signed-off-by: Doug Davis [email protected]