Provide a way to handle 409 Leadership Changed errors in client #1151
Conversation
Without this change, it makes it hard for clients to handle consumer errors relating to leadership changes differently from any other error. Follows same approach as ErrConsumerDeleted.
PR Review feedback over [slack](https://form3.slack.com/archives/C02281GJCJF/p1669986912679049). Currently there are already users who are performing error string comparision for this use case. To avoid introducing a breaking change until there is a better way of handling these error messages, we will retain the error message for the newly introduced canonical error value.
| if strings.Contains(strings.ToLower(string(msg.Header.Get(descrHdr))), "leadership change") { | ||
| err = ErrConsumerLeadershipChanged | ||
| break | ||
| } |
There was a problem hiding this comment.
Thanks, we need to revise this codepath eventually to bubble up these errors in a way that is more predictable... These errors are JetStream errors without an error code so maybe not well represented by the APIError type, what I'm thinking is that we could wrap this as a new statusError type so that below when return the error with an unexpected case.
There are other 409 status codes that we are not catching that may end up in a similar way, probably NATS/1.0 409 Message Size Exceeds MaxBytes will end up as "nats: Message Size Exceeds MaxBytes" for example.
Have a wip on trying to addressing some of this in a different PR: aa8123d
There was a problem hiding this comment.
Thanks, we need to revise this codepath eventually to bubble up these errors in a way that is more predictable... These errors are JetStream errors without an error code so maybe not well represented by the
APIErrortype, what I'm thinking is that we could wrap this as a newstatusErrortype so that below when return the error with an unexpected case.There are other 409 status codes that we are not catching that may end up in a similar way, probably
NATS/1.0 409 Message Size Exceeds MaxByteswill end up as "nats: Message Size Exceeds MaxBytes" for example. Have a wip on trying to addressing some of this in a different PR: aa8123d
Yeah, this PR is just a stop gap measure.
hdr := []byte("NATS/1.0 409 Leadership Change\r\n\r\n")
for reply := range o.prm {
o.outq.send(newJSPubMsg(reply, _EMPTY_, _EMPTY_, hdr, nil, nil, 0))
}
o.prm = nilAt the moment, the message header and body are just defined as byte slices. I would say this isn't very sufficient structure enough when it comes to managing changes to the protocol or supporting a better error handling without inventing a new binary data protocol. Which is of course possible, but it comes as other costs, such as implementing it in all languages there is a client library for the nats libraries to use.
It would be perhaps good to start using a serialising format which would support backwards and forward compatibility, nested data structures and has library support for a number of languages already, such as Protobufs, Avro, Thrift, or whatever that fits Nats eco system best.
Such an approach would even give the possibility of having the error codes explicitly published thus making it very easy for clients to know what errors exist to map them to canonical error types in the language the client library is written in.
|
I'm not authorized to press the merge button, can someone do it please? :) |
Without this change, it makes it hard for clients to handle consumer errors relating to leadership changes differently from any other error.
Follows same approach as ErrConsumerDeleted.