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

Allow AckAll for pull-based consumers #1063

Merged
merged 1 commit into from
Aug 31, 2022

Conversation

neilalexander
Copy link
Member

If you are the only client pulling on a given consumer, the efficiency gains of fetching in batches is largely lost if you have to acknowledge every message you received individually afterwards.

It would seem intuitive to be able to acknowledge only the most recent message in the batch instead and to have that do the right thing.

If you are the only client pulling on a given consumer, the
efficiency gains of fetching batches is largely lost if you
have to acknowledge every message you received individually.
It would seem intuitive to be able to acknowledge only the
most recent message in the batch instead.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 85.557% when pulling bb31b65 on neilalexander:neilalexander/pullackall into c3a557a on nats-io:main.

@aricart
Copy link
Member

aricart commented Aug 31, 2022

On horizontally scaled deployments AckAll would ack messages that are in flight or being processed by different consumers.

@ripienaar
Copy link
Contributor

@aricart same as push+qsubs which we do support.

@kozlovic
Copy link
Member

same as push+qsubs which we do support

@ripienaar Then I think we should not, meaning we should reject an AckAll for a QueueSubscribe() because indeed there may be more than one member and I think it is bad that one member acks messages from other members.

@ripienaar
Copy link
Contributor

@kozlovic since 8 months the server allows this - agree its risky and probably a bad idea.

@kozlovic
Copy link
Member

Indeed, seems like the server all since this PR: nats-io/nats-server#2776, so I guess restriction should be removed in client libraries too... yes, I think it is a bad idea.

@neilalexander
Copy link
Member Author

I can't really comment on the queue subscriptions as not using those, but in the case of one pull subscriber per consumer (as is the pattern that we are using in our application here), AckAll is a pretty big net performance win.

Have done quite a bit of testing with this PR and seems to work fine, although wondering if there might be some sensible place to put a comment warning about the potential for multiple clients to tread on each other.

@aricart
Copy link
Member

aricart commented Aug 31, 2022

@ripienaar @kozlovic While we are at it, I think we also have the restriction that pull consumers must ack, but wondering if we should allow AckNone - this would allow other scenarios like ordered pull consumer etc.

@ripienaar
Copy link
Contributor

For the moment, server would not allow that.

@derekcollison
Copy link
Member

AckAll was put in to support emulated push in clients with efficient ephemeral pull consumers under covers.

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Since the server accepts it, I guess there is no reason to prevent the user to using this ack mode for pull consumers...

@kozlovic kozlovic merged commit 5d4f44e into nats-io:main Aug 31, 2022
@neilalexander neilalexander deleted the neilalexander/pullackall branch August 31, 2022 22:54
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.

6 participants