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.

Sign up for GitHub

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

Jump to bottom

Adding a function to wait for publisher confirms #841

Merged
merged 19 commits into from
Oct 17, 2024
Merged

Adding a function to wait for publisher confirms #841

merged 19 commits into from
Oct 17, 2024

Conversation

manchicken
Copy link

  • Also added an example program to demo it
  • Also removed a deprecated uninit function call from the ssl example

manchicken and others added 5 commits July 24, 2024 16:12
@manchicken
Adding a function to wait for publisher confirms
a4941fb 
- Also added an example program to demo it
- Also removed a deprecated uninit function call from the ssl example
@manchicken
Update amqp_confirm_select.c
d417a11 
Correcting unused lvalue error.
@manchicken
Update amqp_confirm_select.c
758d8b8 
Fixing more unused params issues
@manchicken
Update amqp_confirm_select.c
24c25d7 
Correcting format.
@manchicken
Converting to more portable format macros
8c5412d
@manchicken
Copy link
Author

I'm working on adding publisher confirms to the Net::AMQP::RabbitMQ Perl module, and this seemed like a bit of functionality I could add back upstream. Let me know if I missed anything, or if there's anything you'd like me to update.

The example isn't the most imaginative, I know, but it shows how to use the new function.

@manchicken manchicken force-pushed the adding-confirm-select-example branch from bd822a8 to 5fbeae2 Compare July 25, 2024 03:23
@manchicken
Copy link
Author

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.

@manchicken manchicken mentioned this pull request Jul 25, 2024
Open
@alanxz
Copy link
Owner

alanxz commented Jul 25, 2024

Approach LGTM.

Will do a pass on the actual implementation shortly.

@manchicken
Copy link
Author

Thanks! My C is a bit rusty, so feel free to point out anything I could improve there as well.

alanxz
alanxz requested changes Jul 28, 2024
View reviewed changes
librabbitmq/amqp_api.c Outdated Show resolved Hide resolved
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,
Copy link
Owner

Choose a reason for hiding this comment

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

Could you describe the intended purpose of the envelope parameter?

Seems like this may not be required.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Did you have any more questions or requests for change on this?

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

I like the pulling the channel as an output parameter, and I'll combine that change with the method out param as well. I think making it filter by channel ID may be outside the scope of what I'm doing here, and the channel out param will allow the caller to easily implement that if they want to.

include/rabbitmq-c/amqp.h Outdated Show resolved Hide resolved
examples/amqp_ssl_connect.c Outdated Show resolved Hide resolved
manchicken and others added 2 commits July 31, 2024 19:26
@manchicken
Making changes per PR feedback
080f747 
- Removing extra empty line
- Changing order of params so `in`s come before `out`s.
@manchicken
Merge branch 'master' into adding-confirm-select-example
50f53c3
@manchicken
Copy link
Author

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

@manchicken
Copy link
Author

@alanxz any updates?

alanxz
alanxz requested changes Sep 3, 2024
View reviewed changes
include/rabbitmq-c/amqp.h Show resolved Hide resolved
@alanxz
Copy link
Owner

alanxz commented Sep 3, 2024

@alanxz any updates?

Apologies for the delay, life has been a bit busy lately.

@manchicken
Copy link
Author

Apologies for the delay, life has been a bit busy lately.

Don't I know it, friend. Good luck and solidarity.

@manchicken
Copy link
Author

@alanxz I looked at this a bit more and left some notes. Please let me know what you think.

Also, is there any way we could label this PR hacktoberfest? 'Tis the season.

@manchicken
Refactored publisher confirms code.
e73deb4 
- Added a typedef for publisher confirm responses
- Simplified `amqp_publisher_confirm_wait()` to only have a single out parameter which contains everything the caller would want
- Now `amqp_publisher_confirm_wait()` handles `basic.ack`, `basic.nack`, and `basic.reject`
@manchicken
Removing some unnecessary debug code.
2a145a6
alanxz
alanxz reviewed Oct 7, 2024
View reviewed changes
Copy link
Owner

@alanxz alanxz left a comment

Choose a reason for hiding this comment

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

This looks much better.

I'll fix the fuzzer Check later this week, please resolve remaining small issues and I'll merge this.

librabbitmq/amqp_api.c Outdated
break;

default:
fprintf(stderr, "Got 0x%X for the method, which is «%s».\n",
Copy link
Owner

Choose a reason for hiding this comment

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

The library shouldn't print anything to stderr/stdout.

Please remove.

Copy link
Author

Choose a reason for hiding this comment

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

Ack! Debug code. Sorry, that's what I get for pushing changes after bedtime. I'll get that.

include/rabbitmq-c/amqp.h Show resolved Hide resolved
include/rabbitmq-c/amqp.h Outdated Show resolved Hide resolved
include/rabbitmq-c/amqp.h Show resolved Hide resolved
@alanxz
Copy link
Owner

alanxz commented Oct 7, 2024

#843 should fix the CIFuzz check issue

@manchicken
Copy link
Author

manchicken commented Oct 8, 2024 •
edited
Loading

#843 should fix the CIFuzz check issue

Did you want me to merge that into my branch?

I've updated my branch. I'm gonna try to make the requested changes now.

Do you think you could add the hacktoberfest label to this PR?

@manchicken
Copy link
Author

@alanxz Any updates? I'd like to see if I can get things moving along soon so I can start my work on the Perl side of what I'm doing with this.

@manchicken
Update amqp_confirm_select.c
3b3ce80 
Removing sleep and unused
alanxz
alanxz approved these changes Oct 17, 2024
View reviewed changes
Copy link
Owner

@alanxz alanxz left a comment

Choose a reason for hiding this comment

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

Two minor things, otherwise this is good to go, thanks for your patience.

examples/amqp_confirm_select.c Outdated Show resolved Hide resolved
include/rabbitmq-c/amqp.h Outdated Show resolved Hide resolved
@manchicken
Update amqp_confirm_select.c
34de475 
Removing debug code
@manchicken
Update amqp.h
63bfada 
Updated code doc for accuracy
@manchicken
Copy link
Author

Updated; thanks.

@alanxz alanxz merged commit 7d12118 into alanxz:master Oct 17, 2024
14 checks passed
@manchicken
Copy link
Author

🥳

@manchicken
Copy link
Author

@alanxz Do you know when this will be released? Not trying to rush you, just trying to time things better.

@alanxz
Copy link
Owner

alanxz commented Oct 25, 2024

@manchicken I'll try and roll a release in the next few weeks.

@manchicken
Copy link
Author

manchicken commented Oct 25, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alanxz alanxz alanxz approved these changes

Assignees
No one assigned
Labels
hacktoberfest
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@manchicken @alanxz