add explicit ack/nak for nats jetstream #1073
Conversation
| // see: ManualAck() subscription option | ||
| // see: ConsumerConfig.AckPolicy | ||
| if err != nil { | ||
| _ = m.Msg.Nak() |
There was a problem hiding this comment.
Does ignoring the return here potentially cause issues? Is the 'already ak'd' error the only kind of error that we can get from this operation?
There was a problem hiding this comment.
Added code to handle expected error cases. In all other cases, will return error.
stephen-totty-hpe
force-pushed
the
totty/nats-ack-2024
branch
from
c000d07
to
fa580b6
Compare
Signed-off-by: stephen-totty-hpe <stephen.totty@hpe.com>
stephen-totty-hpe
force-pushed
the
totty/nats-ack-2024
branch
from
fa580b6
to
45ce6e8
Compare
| // see: ManualAck() subscription option | ||
| // see: ConsumerConfig.AckPolicy | ||
| if err != nil { | ||
| errNak := m.Msg.Nak() |
There was a problem hiding this comment.
What are other errors that Ack and Nak can return? Can you please run a local dummy test with a real NATS backend and always return an error here so we understand how the system behaves end-to-end? IMHO it should be fine (warning logged), but want to make sure this doesn't lead to an endless invocation loop since with this change we're changing the behavior for users.
There was a problem hiding this comment.
I will close this PR if #1095 gets merged.
In some cases, it is necessary to control manual ACK/NCK based on the existance of errors. It is ok to ACK/NCK even if the message has already been committed. In that case, the ACK/NCK is short-circuited.