search

golevelup / nestjs

A collection of badass modules and utilities to help you level up your NestJS applications 🚀
MIT License
2.27k stars 261 forks source link

Separate publisher/consumer connections for rabbitmq #679

Open ttshivers opened 9 months ago

ttshivers commented 9 months ago

According to best practices, it is good to use separate connections entirely for publishers and consumers so RabbitMQ can apply backpressure on the tcp connections if the publisher is sending too many messages but still allow consumers to operate properly and send back acks.

https://www.cloudamqp.com/blog/part1-rabbitmq-best-practice.html#separate-connections-for-publisher-and-consumer

underfisk commented 9 months ago

@ttshivers I like this idea! Is there any action item/follow up that us, the library Authors can do? Updating documentation? Improving the module API? Feel free to propose and i'd be happy to review/discuss

ttshivers commented 9 months ago

I think this could be accomplished with an internal change where the publish method uses a separate connection that we would also need to set up.

underfisk commented 9 months ago

I think this could be accomplished with an internal change where the publish method uses a separate connection that we would also need to set up.

Do you have a change proposal in mind or something you could show off in a PR? I haven't used the rmq package in awhile so I'm open to suggestions

ttshivers commented 9 months ago

I think this could be accomplished with an internal change where the publish method uses a separate connection that we would also need to set up.

Do you have a change proposal in mind or something you could show off in a PR? I haven't used the rmq package in awhile so I'm open to suggestions

Sure. Also, I think this may involve changing the names of many public properties to properly differentiate between the consumer and producer connections.

https://github.com/golevelup/nestjs/blob/8962eed4ce527dba11fe7799de58cdf33d066e52/packages/rabbitmq/src/amqp/connection.ts#L147 would become consumerConnection and there would also be a publisherConnection.

underfisk commented 9 months ago

I think this could be accomplished with an internal change where the publish method uses a separate connection that we would also need to set up.

Do you have a change proposal in mind or something you could show off in a PR? I haven't used the rmq package in awhile so I'm open to suggestions

Sure. Also, I think this may involve changing the names of many public properties to properly differentiate between the consumer and producer connections.

https://github.com/golevelup/nestjs/blob/8962eed4ce527dba11fe7799de58cdf33d066e52/packages/rabbitmq/src/amqp/connection.ts#L147

would become consumerConnection and there would also be a publisherConnection.

I like that idea! I'm not against a change of API if it's for the better, again this API should evolve and there's room for improvement

ttshivers commented 9 months ago

One thing I'm considering as well is cases where only consumption is used so a publisher channel wouldn't be needed.

Separate connections can be created with the current state by registering the module multiple times so perhaps that might be the way to go.

underfisk commented 9 months ago

A while ago i had the idea of "connection pooling" because it felt that some connections were being recreated and could potentially be re-used but if you do have an RFC feel free to propose, don't feel limited

irarayjenkins commented 5 months ago

Has there been any movement on this?

github-actions[bot] commented 2 weeks ago

This issue is stale because it has been open for 30 days with no activity.