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

Redeliver publisher confirms acks again and again from 0 till current tag #615

Closed
kinnalru opened this issue May 23, 2021 · 4 comments · Fixed by #617
Closed

Redeliver publisher confirms acks again and again from 0 till current tag #615

kinnalru opened this issue May 23, 2021 · 4 comments · Fixed by #617

Comments

@kinnalru
Copy link
Contributor

I wrote some benchmarks and met some strage behaviour:
In confirm_select mode sometimes callback called with already acked delivery tag. I start to investigate and find this code:
https://github.com/ruby-amqp/bunny/blob/master/lib/bunny/channel.rb#L1792

When rabbitmq sends me an confirm with multiple equals to true confirmed_range_start setted to @delivery_tag_offset + 1. But @delivery_tag_offset is always 0 without network recoveries!

So I have such results:
Send 16000 very small messages
ConfirmSelect callback called 119567 times - each delivery_tag was acked ~ 9 times.

Which workaround can I use? The main fix I see to pass tag|multiple|nack triplet to user in callback without changes, avoiding useless work to iterate through tags from beginning over and over

@michaelklishin
Copy link
Member

I don't see a solution that would not involve keeping track of the "most recently seen" delivery tag which would be reset upon connection recovery. Then we'd use that as the lower range boundary. How does that sound?

I'm not sure if most users would appreciate being exposed to the multiple property value. That would also be a breaking public API change. It should be very rare to see a confirmation of many (say, dozens or hundreds) of messages at a time,
so invoking the callback once for every delivery still seems like a reasonable design choice to me.

@kinnalru
Copy link
Contributor Author

kinnalru commented May 24, 2021

Hi @michaelklishin
With low requests frequency RabbitMQ don't use multiple attribute on acks.
But sometimes our service serves about 500 rsp. and RabbitMQ sometimes sends multiple attribute. So after a while without network failures we receive dozens of confim_select callback for delivery_tag most of which was succesefuly acked some hours ago ex: from 0 to 16k acks. :(

Are you intresting in PR with little complex logic in handle_ack_or_nack ? I testing draft impl now.

@michaelklishin
Copy link
Member

I know when RabbitMQ would confirm N messages at once: when there are several of them confirmed by target queues since the last time a delivery tag was sent out. So indeed it would take a certain ingress message rate.

I would be interested in a PR that makes handling of basic.acks with multiple set to true correctly.

kinnalru added a commit to RND-SOFT/bunny that referenced this issue Jun 14, 2021
Don't reconfirm already acked messages when multiple is true. Fixes ruby-amqp#615
@kinnalru
Copy link
Contributor Author

I would be interested in a PR that makes handling of basic.acks with multiple set to true correctly.

Morning.
We create PR for this issue.

@michaelklishin please give us some comments

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 a pull request may close this issue.

2 participants