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

feat: add frameMax connection option #57

Merged
merged 1 commit into from
Sep 21, 2024

Conversation

dwickr
Copy link

@dwickr dwickr commented Apr 26, 2024

Add the frameMax connection tuning parameter as implemented in amqplib1. Default to 4k (4096) to match amqplib's default.

Add the `frameMax` connection tuning parameter as implemented in
amqplib[1]. Default to 4k (4096) to match amqplib's default.

[1]: https://amqp-node.github.io/amqplib/channel_api.html#tuning-parameters
@dwickr
Copy link
Author

dwickr commented Jun 28, 2024

hey @zlintz would you mind reviewing this one? thank you!

Copy link
Member

@zlintz zlintz left a comment

Choose a reason for hiding this comment

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

I think the implementation is correct, the only change I would suggest is making it optional to be consistent with past behavior rather than a default from our side.

Consulting the great GPT oracle with a grain of salt of course, indicated

The default size for frameMax in RabbitMQ is 131072 bytes, which is equivalent to 128 KB.

vs the 4096 bytes being proposed as the default

@dwickr
Copy link
Author

dwickr commented Aug 30, 2024

@zlintz thanks for the feedback. I can look into changing this to be optional instead. The current default of this value set by amqplib -- and therefore this package -- is 4096 bytes. I can increase it to the RabbitMQ default of 128KiB, but that would be a slight change in behavior here.

@zlintz
Copy link
Member

zlintz commented Aug 30, 2024

Ah I see what you mean https://github.com/amqp-node/amqplib/blob/64d1c1ec19afa64a7ec5c355ea7620f0b227fb30/lib/connect.js#L69

In this case, I don't think you need to adjust your proposal. I will try and get this merged in and released.

Thank you for contributing

@zlintz zlintz merged commit 8ca9b46 into Foo-Foo-MQ:main Sep 21, 2024
2 checks passed
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 this pull request may close these issues.

2 participants