ssi-anik / laravel-amqp

anik/amqp wrapper for Laravel-ish frameworks
https://packagist.org/packages/anik/laravel-amqp
MIT License
29 stars 4 forks source link

Add support for laravel octane #12

Closed abublihi closed 2 months ago

abublihi commented 2 months ago

This pull request is related to https://github.com/ssi-anik/amqp/issues/31, as this simple change will fix the issue while using laravel octane,

This change will make laravel only resolve the connection once after the application boots (warm), the user of the package should add the connection to warm parameter in Octane configuration file. ( this been added to readme )

Making only one connection to rabbitmq for each Worker of laravel octane will boost the performance.

Best regards.

abublihi commented 2 months ago

@ssi-anik have you checked the PR?

ssi-anik commented 2 months ago

@abublihi Assalamu alaikum brother. Yes, I saw the PR the next day you submitted it. I was so busy that I couldn't reply to you. I am sorry for that.

Firstly, the consumer change is not necessary. Because the consumer is only booted once when you run the command and it keeps running. So, the connection issue for the consumer is not a problem for each consumption. Also, as you're keeping the reference of the consumer when passing options to the different consumers, it will return the old consumer as you have implemented if I am not wrong.

Secondly, for the producer, I am currently checking it. trying to make sure that it doesn't break. I will keep you posted here by tomorrow EoD insha-Allah.

abublihi commented 2 months ago

وعليكم السلام ورحمة الله وبركاته،،

Thank you brother @ssi-anik for Reviewing the PR,,

For the consumer you are right I will discard the change, but for the producer I have a question does php-amqplib reconnect if the connection is disconnected for some reason?

ssi-anik commented 2 months ago

@abublihi I am again sorry for the delayed response. I am having trouble managing my time.

So, please remove the consumer connection related changes, I will be merging it and then will work on the broken connection issue if possible.

Thanks for the PR.