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

Issue305 | Add Topic Configurations #19

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tobiasjaster
Copy link

@tobiasjaster tobiasjaster commented Apr 13, 2023

Fix ThreeDotsLabs/watermill#305
Rework the TopologyBuilder to accept individual QueueBinding namings (TopologyBuilder Interface Change!)
Add configurations for topic routing

@m110 can you please review and approve this PR?

@haritzsaiz
Copy link

haritzsaiz commented Apr 24, 2024

Any update on this? I think the Topology Builder change is quite necessary

@darri89
Copy link

darri89 commented May 28, 2024

+1 on this, I would also like to see this change implemented. The current TopologyBuilder does not allow for wildcard matching on topic exchanges.

@tobiasjaster
Copy link
Author

Unfortunately, I no longer use amqp, which is why I'm not putting much effort into it at the moment. However, the merge is currently not down to me but to the lack of a review, if I can see that correctly.

@darri89
Copy link

darri89 commented May 31, 2024

@m110 is it possible to get a review on this PR? I see no way currently with the package to use RabbitMQ topic exchanges with wildcard routing key matching.

pkg/amqp/config.go Outdated Show resolved Hide resolved
type TopologyBuilder interface {
BuildTopology(channel *amqp.Channel, queueName string, exchangeName string, config Config, logger watermill.LoggerAdapter) error
BuildTopology(channel *amqp.Channel, queueName string, exchangeName string, routingKey string, config Config, logger watermill.LoggerAdapter) error
Copy link
Member

Choose a reason for hiding this comment

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

This changes the public interface, so we would need to bump the major version to do this.

Copy link
Author

Choose a reason for hiding this comment

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

That is absolutely correct. Do you have a suggestion on how to solve the routing problem without breaking change? Otherwise, the only solution would be to create your own TopologyBuilder and set an internal variable before each BuildTopology call.

On the other hand, is the handling of routing within the BuildTopology function not so wrong?

For me at least, the PR would no longer be a priority because we have switched to another broker, but the problem seems to exist due to the requests from other developers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants