opened
rcillo
username_0
In order to more precisely communicate partial success scenarios, we are
introducing 207 Multi-status code as a possible http response code.

In case all events have been published successfully, then we just return
200 with empty body, otherwise we provide detailed status information in
the form of an array of BatchItemResponse's.

We considered using 422 as a response answer but it just doesn't feel correct
since we could have a mix of successes and failures in the batch.
aruha-113 add multi-status code 207
issue
created
review-ninja
username_1
<a href="https://app.review.ninja/zalando/nakadi/pull/179" target="_blank"><img src="https://app.review.ninja/44234368/pull/179/badge" alt="ReviewNinja"/>
comment
created
v-stepanov
username_2
I still think that 422 is a an appropriate answer when not a single event was published. So that we can have
- 200 when everything successful
- 207 when some are published and some are failed
- 422 when no event is published successfully

@username_0 @username_3 what do you guys think?
comment
created
jkliff
username_3
@username_2 I don't think so, but maybe I am not seeing why you think so. Do you mind explaining the reasoning behind it?
comment
created
rcillo
username_0
@username_2 I think it makes sense if all of the failures are due to client error. The same way, if all of the events fail due to kafka being down, I think i could be 503, for example.

This is an important discussion. I think that the answer to @username_2 question will have as consequences the removal of other statuses, like 503, for example.
comment
created
v-stepanov
username_2
@username_3 @username_0 
Well, for me everything that starts with 2xx is considered to be a "successful", and if everything failed - there's not much success in that. However in the documentation of 207 it says that you can even use in in failure situations. (https://httpstatuses.com/207)
`The response MAY be used in success, partial success and also in failure situations.`
So in general you are right
.
But I also think that consulting with API guild can also be useful in this case.
comment
created
rcillo
username_0
@username_2 I just realised that it can be useful to reserve 422 for complete failure. For example, let's suppose there's an error on the desserialization of the RequestBody. This error is not "localized" to a single event and should result in a complete failure with 422.

So if we state that 422 should come together with a list of BatchItemResponse's then we have no way to express errors that are not specific to a single event.
comment
created
tfrauenstein
username_4
If think it depends on the failure type. I would 1st check all client side failures, like producing of event that does not match its type scheme. If there is any client failure event in the batch I would react "atomic" with client error status code (e.g. 422) plus return problem object array providing the details and no state change. If it is during server side state change processing phase, I would return either 200 for complete success or 207 for partial state change and success complemented with return problem object array providing the details.
comment
created
jkliff
username_3
Except for failures directly related to the underlying broker (i.e. after validating, etc), all issues are treated atomically (no state change). 
Any case of success and failure will always produce item-by-item responses. 
I agree with @username_2 and @username_4. In being a 4xx, 422 indicates that the opreation was not successful as a whole and this is a simple indication to the client that it wasn't correct. 
200 works as a simple indicator that the whole was successful and 207 serves any situation where some items only were not successful (server side failure in @username_4 terms).

This fits directly with @username_2's proposal. 

I support.
comment
created
dehora
username_5
Just so I understand, what would the return codes be in these cases posting a batch containing two events, A and B - 

- A passes the EventValidationStrategy, and B fails

- Both A and B fail the EventValidationStrategy

- A can be sent onwards to its partition, and B can't

- Both A and B can't be sent to their partitions
comment
created
rcillo
username_0
Response status 207 and response body `[ BatchItemResponse A { step: PUBLISHING, publishing_status: FAILED, detail: "the specifics of the publishing error" }, BatchItemResponse B { step: PUBLISHING, publishing_status: FAILED, detail: "the specifics of the publishing error" } ]`
comment
created
dehora
username_5
@username_0 :+1: thanks
comment
created
rcillo
username_0
Thanks to @username_4 and @username_5 for the feedback. I'll merge this PR.
comment