Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(2.11) Adds replies for AckAck requests #6198

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

jnmoyne
Copy link
Contributor

@jnmoyne jnmoyne commented Dec 2, 2024

Now returns a JS API payload rather than an empty message in response to AckAck requests.
New error: when trying to ack a sequence number higher than the stream's last sequence and when trying to ack a sequence number more than once.

@jnmoyne jnmoyne requested a review from a team as a code owner December 2, 2024 19:29
server/consumer.go Outdated Show resolved Hide resolved
server/consumer.go Outdated Show resolved Hide resolved
server/consumer.go Outdated Show resolved Hide resolved
server/jetstream_test.go Outdated Show resolved Hide resolved
server/jetstream_cluster_1_test.go Outdated Show resolved Hide resolved
jnmoyne added a commit to nats-io/nats-architecture-and-design that referenced this pull request Dec 3, 2024
nats-io/nats-server#6198
Signed-off-by: Jean-Noël Moyne <jnmoyne@gmail.com>
@jnmoyne
Copy link
Contributor Author

jnmoyne commented Dec 3, 2024

This change improves the ability to extend some amount of 'exactly once processing' to JS in that you may want to know if a message was already acknowledged or not by the time you synchronously acknowledge the message (e.g. your process could have been blocked or suspended or disconnected or slow enough to trigger a message re-delivery because of ack wait and that re-delivered message gets processed and acked by someone else before you get to your ack call).

For example the processing of a message results in an SQL database table being updated you could avoid the failure scenario described above by doing in your callback: your processing, create a transaction on the DB and call prepare on it, if prepare is ok then do a syncAck on the message, if it returns successfully then commit the transaction otherwise roll it back. Admittedly, this is not perfect as if the sync ack request times out unfortunately you do not know the ack request or it's response was lost so you have to resort to some kind of exception in case of a timeout, but I think it's still a valuable improvement from the current behavior.

@ripienaar
Copy link
Contributor

While I agree this is useful, I think the specifics of the mechanic where old acts are being honoured largely cancels out gains here if we do not also address that.

So imo, we should address the problem as a whole rather than put small patches.

@jnmoyne
Copy link
Contributor Author

jnmoyne commented Dec 4, 2024

You mean stop accepting ack requests that indicate in their subject that they came from a delivery that is not the latest delivery? I also don't like that but I don't see this change here as incompatible with that (i.e. would still need to start returning a response with payload and there would still be the same 'not pending' error message). Meaning that we could go with this right now and then add later the fact that it will start returning that error if you try to ack a message that has been re-delivered since you received it. (just IMHO)

@ripienaar
Copy link
Contributor

Yes those, my point is I also agree the changes here are compatible with such a change. I am asking that when reworking acts rather than a small piece meal approach to making tiny adjustments we look - probably during 2.12 - holistically at what we learned about acts and how we can improve the overall story.

What appears to be a tiny change quickly snow balls in the face of many clients - we would at the very least have to audit all our clients against this change. Already quite a big ask. Holistically looking at the problems in this space all at once is better imo.

Now returns a JS API payload rather than an empty message in response to AckAck requests.
New error: when trying to ack a sequence number higher than the stream's last sequence and when trying to ack a sequence number more than once.

Signed-off-by: Jean-Noël Moyne <jnmoyne@gmail.com>
Signed-off-by: Jean-Noël Moyne <jnmoyne@gmail.com>
Signed-off-by: Jean-Noël Moyne <jnmoyne@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants