Adding a function to wait for publisher confirms #841
Conversation
manchicken
commented
Jul 24, 2024
manchicken
commented
- Also added an example program to demo it
- Also removed a deprecated uninit function call from the ssl example
- Also added an example program to demo it - Also removed a deprecated uninit function call from the ssl example
Correcting unused lvalue error.
Fixing more unused params issues
Correcting format.
|
I'm working on adding publisher confirms to the The example isn't the most imaginative, I know, but it shows how to use the new function. |
manchicken
force-pushed
the
adding-confirm-select-example
branch
from
bd822a8
to
5fbeae2
Compare
|
Apologies for all of the confusion and CI runs, I don't have a Windows test environment. I think I got all CI passing now. |
|
Approach LGTM. Will do a pass on the actual implementation shortly. |
|
Thanks! My C is a bit rusty, so feel free to point out anything I could improve there as well. |
include/rabbitmq-c/amqp.h
Outdated
| */ | ||
| AMQP_EXPORT | ||
| amqp_rpc_reply_t AMQP_CALL amqp_publisher_confirm_wait( | ||
| amqp_connection_state_t state, amqp_envelope_t *envelope, |
There was a problem hiding this comment.
Could you describe the intended purpose of the envelope parameter?
Seems like this may not be required.
There was a problem hiding this comment.
Yeah, it's the only thing that'll tell you which channel the ACK came through. The code for the amqp_basic_ack_t is correct per spec, naturally, but it can be helpful to know which channel the ACK came on.
There was a problem hiding this comment.
Did you have any more questions or requests for change on this?
There was a problem hiding this comment.
If all you need is the channel id, then use amqp_channel_t* as an output parameter.
Alternatively consider using an amqp_channel_t as an input parameter, then only wait for the confirm on this channel.
| @@ -95,7 +95,6 @@ int main(int argc, char const *const *argv) { | |||
| die_on_amqp_error(amqp_connection_close(conn, AMQP_REPLY_SUCCESS), | |||
| "Closing connection"); | |||
| die_on_error(amqp_destroy_connection(conn), "Ending connection"); | |||
| die_on_error(amqp_uninitialize_ssl_library(), "Uninitializing SSL library"); | |||
There was a problem hiding this comment.
Deprecation notices complaining when building examples.
There was a problem hiding this comment.
Please send me a separate PR for this change.
- Removing extra empty line - Changing order of params so `in`s come before `out`s.
|
@alanxz Let me know if there's anything else you'd like to see changed here. I'd really like to get this into my Perl module, users have been asking for it for a long time. |
|
@alanxz any updates? |
| * \returns amqp_rpc_reply_t * | ||
| */ | ||
| AMQP_EXPORT | ||
| amqp_rpc_reply_t AMQP_CALL amqp_publisher_confirm_wait( |
There was a problem hiding this comment.
Thinking through this API a bit more we need to consider a couple different scenarios:
- What does this API do if the connection isn't in publisher-confirm mode? From an API-use perspective, it'd be helpful to return some kind of error if the connection isn't correctly configured.
- In publisher confirm mode, when a message is published the broker will return one of basic.ack, basic.nack, basic.reject (with channel.close and connection.close as options, but already handled). (see: https://github.com/alanxz/SimpleAmqpClient/blob/500b070465f7759f0bf984ff56b724c06df930ef/src/Channel.cpp#L837-L877). From API-use perspective, it'd be helpful if this function handled all 3 cases, likely this means: this function should have as an out parameter an
amqp_method_number_t, and avoid*, which is a library allocated version of one of the previously referenced methods.
Apologies for the delay, life has been a bit busy lately. |
Don't I know it, friend. Good luck and solidarity. |
