Add clarity around concurrent updates

[duglin]

From the f2f: Proposal: In the “Blocking Operations” section change it to say “...that mutate the same resource…” instead of “...that act on the same set of resources...”

&& creating Binding is not necessary considered mutating an Instance, it’s a Broker’s choice

&& move “Blocking Operations” out from under Async

&& change 422 in createBinding response status code table to include this concurrency stuff

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

[cfdreddbot]

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.

[angarg12]

LGTM

[jmrodri]

@duglin PR title has a TYPO: conurrent should be concurrent

[duglin]

@jmrodri fixed! thanks

[avade]

LGTM

[mattmcneeney]

LGTM

[duglin]

just one more review needed

[jmrodri]

Not sure I count but LGTM :)

[duglin]

Added text about orphan mitigation per the weekly meeting.

[duglin]

I changed the error code from 422 to "409 Conflict" - seemed more appropriate, but what do others think?

[jmrodri]

@duglin 409 Conflict does sound more appropriate. 👍

[angarg12]

LGTM

[duglin]

reviews needed

[duglin]

updated and ready for reviews

[mattmcneeney]

LGTM

[angarg12]

LGTM

[duglin]

ready for next round of reviews

[mattmcneeney]

LGTM

[fmui]

The proposal defines a fixed error body. A broker can neither change the description nor add additional data that might be useful to identify why there is a conflict. Can we make this a bit more flexible?

[duglin]

@fmui See: https://github.com/duglin/servicebroker/blob/6cbba3d423f3d7e5ebadd2e650f245ce531e1e2f/spec.md#broker-errors it says: "For error responses, the following fields are valid. Others will be ignored. ". While it would be nice if it was a bit clearer, I view that as saying the broker may include other fields (extensions) but they might be ignored by the platform. Does that cover it?

[fmui]

Not really. The proposal says: "...then the broker MUST reject the request with an HTTP 409 Conflict and the following body: ..."

This is much more restrictive than defined in the Broker Error section. It doesn't even allow to return a different description text. I don't see a reason why this error should have stricter rules than other errors.

[duglin]

I don't think it was meant to be more restrictive. I think an generic "error PR" to make us more consistent would be good to answer these concerns. In particular:

• which fields in the error json MUST be as exactly specified or can be tweaked (e.g description)
• are additional fields (extensions) allowed and how are they added? (wrapper or top-level)

and we should touch all error text to ensure consistency.

[vaikas]

+1 to allowing for additional fields. We have already run into some issues because brokers are built on platforms that provide some fixed fields for all error responses.

[duglin]

@fmui can we clarify all of this in a follow-on PR? I'd prefer to keep this one more focused on just this one usecase.

[zrob]

I have a few concerns here.

1. changing the MUST from 422 to 409 is a backwards incompatible change
2. similar to fmui concern, this feels like a very restrictive error message, where brokers may want to include more or different details that could be helpful in resolving the issue
3. I don't understand the benefits of making this change. I can see that 409 may be somewhat more correct, but do we have platforms that will perform a different behavior based on this? As long as we are returning an error and a useful message I don't see a reason to be more prescriptive than we already are.

[duglin]

I reverted back to 422 and decided to add the "extensibility" text into this PR instead of creating a new one. See what people think.

[mattmcneeney]

@duglin merge conflict!

[duglin]

rebased

[mattmcneeney]

This is going to give me merge conflict fun times in the async bindings PR. But LGTM anyway!

[angarg12]

LGTM

[fmui]

LGTM

[avade]

lgtm