Amqp: Publish method does not return "confirms.Published" counter(deliveryTag)

Created on 29 Sep 2020 · 9Comments · Source: streadway/amqp

I have a listener registered through NotifyPublish and it is asynchronously getting confirmation and if ack=false then I republish the message or act upon it, but I get delivery tag from this listener channel and I do not know which payload this delivery tag is of.

While looks like we can return the https://github.com/streadway/amqp/blob/v1.0.0/confirms.go#L36 which looks like the delivery tag for the given channel, but we are ignoring it in Publish method https://github.com/streadway/amqp/blob/v1.0.0/channel.go#L1360.

Source

kamal-github

All 9 comments

Hi @kamal-github

I was wondering the same, until I remembered that confirmations are sent on the channel where I publish, only for what I publish on that channel...

ie. confirmation notification with DeliveryTag=1 is for the first message you published on the channel, etc... It's easy to keep track of it (but I agree, Publish could have returned that counter).

chrisDeFouRire on 6 Oct 2020

👍1

@chrisDeFouRire I am worried about maintaing the same counter which might have concurency issue. How about adding a method like Java client has. getNextSequenceNumber() ? https://www.rabbitmq.com/tutorials/tutorial-seven-java.html

kamal-github on 9 Oct 2020

@kamal-github I like your solution, though I can only give it my vote 👍

chrisDeFouRire on 10 Oct 2020

👍1

This client uses a really conservative approach when it comes to even theoretically breaking public API changes. Exposing a sequence counter reader — is the way to go.

michaelklishin on 15 Oct 2020

Please submit a PR that introduces a new function that returns the counter, or a doc update if it already exists. Breaking Channel.Publish modifications (such as the return value) are out of the question for this client.

michaelklishin on 15 Oct 2020

@michaelklishin I think that's exactly what PR #478 (by kamal-github) does

chrisDeFouRire on 15 Oct 2020

@michaelklishin As @chrisDeFouRire mentioned that https://github.com/streadway/amqp/pull/478 I have already created the PR for this. Please review. :)

kamal-github on 17 Oct 2020

@michaelklishin

When merging it. https://github.com/streadway/amqp/pull/478