Add NATS Ack/Nak to nats jetstream V3 Finish

[stephen-totty-hpe]

In some cases, an explicit ACK/NACK may be necessary. Currently, there is no way to NACK a message. Also, in some cases, the NATS server may not automatically ACK a message.

This logic explicitly looks for an error that is protocol.ResultNACK, for when to do a Nak.
If the error is nil or protocol.ResultACK, then a Ack is done.
When an unknown error occurs, the original behavior of return nil is invoked.

Testing with a live system can be done by temporarily modifying:
https://github.com/cloudevents/sdk-go/blob/main/v2/protocol/test/test.go#L41
that is called from:
https://github.com/cloudevents/sdk-go/blob/main/test/integration/nats_jetstream/v3/nats_test.go#L29

[embano1]

So we're slurping the error and returning nil? Is this the correct behavior when finishing?

[stephen-totty-hpe]

This is the previous behavior. I am open to changing this behavior if there is support to change it.

[embano1]

Got it, good question if we want consistency (but ambiguous behavior) or fix it. What do you think? Perhaps someone using v3 would not use the prior implementations and therefore we should do what's right for users?

[stephen-totty-hpe]

After looking at
https://github.com/cloudevents/sdk-go/blob/main/v2/client/client.go#L263
and
https://github.com/cloudevents/sdk-go/blob/main/v2/client/invoker.go#L59

I believe the error returned from Finish is only used for logging:
cecontext.LoggerFrom(ctx).Warn("Error while handling a message: ", err)

So the real question is more about whether we should call Ack() or Nak() in the unknown error case.
In the case that Ack() or Nak() is not called, the ACK/NACK behavior on the server is based on how the NATS consumer is setup.
There are three options: AckNonePolicy, AckAllPolicy, AckExplicitPolicy

So I would think if you had AckNonePolicy or AckAllPolicy, not calling Ack() would result in the server auto-acking.
If you had AckExplicitPolicy set, then failing to call Ack() would result in the message being redelivered.

This might be correct as it would "default" to how the user setup the consumer.
NATS has a MaxDeliver option which would give up after the sending "MaxDeliver times", but the default is "unlimited".

I am inclined to give the user the benefit of defining that behavior, but I am not married to that feeling.

[embano1]

Yeah, that sounds reasonable! Feel free to document the behavior e.g., adding a README or code comments.

[stephen-totty-hpe]

Added a comment.

[embano1]

Interesting, there's a case when a (protocol) NACK would contain NATS ACK-ed in this implementation?

[stephen-totty-hpe]

nak and ack make the same NATS call deep down with a minor difference in the ackType argument passed via the API. The ErrMsgAlreadyAckd error that comes back simply means that the message has been already "marked" as having been processed. It is used for both ack and nak.

In practice, a message should not be marked ack or nak before calling Finish. But there may be cases where a binding may have a behavior to handle MalformedEvent or a responder may be interjected. In these cases, I believe Finish will still only called once. The error check is simply there to not send back an error in the unlikely case somehow Finish is called more than once.