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

[DBZ-7909] RabbitMQ Native Stream: Making sure RequestedFrameMax get sent to StreamClient #119

Closed
wants to merge 3 commits into from

Conversation

zikphil
Copy link
Contributor

@zikphil zikphil commented Sep 9, 2024

This fixes an issue I was having where I was setting the frame_max variable on the connection configuration but it was not propagating to the StreamClient. This patch fixes this issue.

https://issues.redhat.com/browse/DBZ-7909

@zikphil zikphil changed the title DBZ-8201: Making sure RequestedFrameMax get sent to StreamClient [DBZ-8201] RabbitMQ Native Stream: Making sure RequestedFrameMax get sent to StreamClient Sep 9, 2024
@zikphil zikphil changed the title [DBZ-8201] RabbitMQ Native Stream: Making sure RequestedFrameMax get sent to StreamClient [DBZ-7909] RabbitMQ Native Stream: Making sure RequestedFrameMax get sent to StreamClient Sep 9, 2024
@jpechane
Copy link
Contributor

@zikphil Could you please change the commit message as it refers to DBZ-8201 instead of DBZ-7909

@jpechane
Copy link
Contributor

@zikphil LGTM, would it be possible to add the remaining config options of the EnvironmentBuilder?

This fixes an issue I was having where I was setting the frame_max variable on the connection configuration but it was not propagating to the StreamClient. This patch fixes this issue.
@zikphil
Copy link
Contributor Author

zikphil commented Sep 10, 2024

@zikphil LGTM, would it be possible to add the remaining config options of the EnvironmentBuilder?

@jpechane This is the part that is a bit unclear to be. I do not code Java but it appears to me that the ConnectionFactory is not used at all by the rabbitmq-stream-client so I am not sure why this is part of the debezium-server documentation and used in the class itself. Perhaps it is by convenience just to get validation on the host/port/virtualHost etc... but none of the wildcard will end up being used by the EnvironmentBuilder unless coded in.

I added a new commit that removes the usage of the ConnectionFactory and instead uses the variables documented here: https://rabbitmq.github.io/rabbitmq-stream-java-client/stable/htmlsingle/#configuring-the-environment

I've also added some config for the StreamProducer defined here: https://rabbitmq.github.io/rabbitmq-stream-java-client/stable/htmlsingle/#producer

It is a bigger change, perhaps we could squeeze it in for 3.0 Beta. If you accept this I can also put a PR for the documentation to be updated.

@jpechane
Copy link
Contributor

jpechane commented Oct 1, 2024

@zikphil Hi, I am sorry for the late reply. I think the PR is good. I'd just keep the old property names for host/port (with connection prefix), makr them as deprecate and emit a warning when used. They'd be removed later. The docs update will reflect the new names and new config options. WDYT?

@zikphil
Copy link
Contributor Author

zikphil commented Oct 1, 2024

@jpechane No worries! Im not sure if I've done this right, is that what you were looking for?

@jpechane
Copy link
Contributor

jpechane commented Oct 9, 2024

@zikphil Thanks a lot, applied via #131

Could you please also send a docs PR to document the renamed and new config options?

@jpechane jpechane closed this Oct 9, 2024
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