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

Publish method does not return "confirms.Published" counter(deliveryTag) #476

Closed
kamal-github opened this issue Sep 29, 2020 · 10 comments · May be fixed by #478
Closed

Publish method does not return "confirms.Published" counter(deliveryTag) #476

kamal-github opened this issue Sep 29, 2020 · 10 comments · May be fixed by #478

Comments

@kamal-github
Copy link

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.

@kamal-github kamal-github changed the title Publish method does not return confirms.Published counter(deliveryTag) Publish method does not return "confirms.Published" counter(deliveryTag) Sep 29, 2020
@chrisDeFouRire
Copy link

chrisDeFouRire commented Oct 6, 2020

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).

@kamal-github
Copy link
Author

@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

@chrisDeFouRire
Copy link

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

@michaelklishin
Copy link
Collaborator

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
Copy link
Collaborator

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.

@chrisDeFouRire
Copy link

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

@kamal-github
Copy link
Author

@michaelklishin As @chrisDeFouRire mentioned that #478 I have already created the PR for this. Please review. :)

@qiaohongbo
Copy link

@michaelklishin

When merging it. #478

@qiaohongbo
Copy link

@michaelklishin As @chrisDeFouRire mentioned that #478 I have already created the PR for this. Please review. :)

When merging it

@samyonr
Copy link

samyonr commented Nov 18, 2021

For everyone who reaches this thread now, note that it was fixed in #478 (but not released yet) and the exact same commit was released in rabbitmq/amqp091-go: https://github.com/rabbitmq/amqp091-go/releases/tag/v1.2.0

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.

5 participants