search

bschmitt / laravel-amqp

AMQP wrapper for Laravel and Lumen to publish and consume messages
MIT License
268 stars 86 forks source link

add rpc #54

Open gitkv opened 5 years ago

gitkv commented 5 years ago

add default workers edit readme

see https://github.com/bschmitt/laravel-amqp/issues/9#issuecomment-404814555

stevenklar commented 5 years ago

Hi @gitkv Thanks for your contribution.

There seems to be a lot of changes in one single commit.

I will try review this very soon. Please think about splitting this PR more context aware. After a quick glance I noticed three different changes:

Review multiple smaller PRs with a specific context is easier to read and possibly faster to discuss/merge.

-Steven

gitkv commented 5 years ago

Hi! I shared a commit. Many people in the community, on the contrary, ask to make squash commits, so I immediately sent 1 commit.

Sorry for my English, I am Russian, I write with google translate)))

bschmitt commented 5 years ago

@gitkv Thank you for your contribution!

I'm gonna have a look to all the changes as well. Bear with me, it might take some days.

Have a good weekend!

bschmitt commented 5 years ago

@gitkv your english is great, so no need to apologize, we're all non native speakers.

I finally found some time to check your PR, the RPC implementation is really nice, if you don't mind, I'd only want to change some minor things.

What I'm a bit concerned about is the worker approach. I built that library to offer some low level, easy to use and most flexible AMQP support for Lumen and Laravel, as the Laravel's queue implementation was targeting to in-app messaging only. In my case I needed something which is able to interact with totally different systems build in different programming languages.

With your implementation, of course it would still be possible to use regular consumers (instead of workers which trigger events) but I'm not sure if it makes sense in case of flexibility and ease of use.

Currently I'm free to use consumers in the way I need, e.g. trigger events or do direct model modifications, they can be executed everywhere e.g. in commands or http actions.

Maybe I misinterpreted the concept of workers? Did you had something different in mind when building them? Don't get me wrong, I just need some clarification.

@stevenklar what is your opinion?

stevenklar commented 5 years ago

After review everything here are my side notes additional to my code review notes:

The worker you implemented seems like an "EventConsumer" which is a consumer implemented in the laravel event system. Which is pretty nice but I'm with @bschmitt that is might be confusing as this repository for me tries to be the layer between amqp and laravel. This implementation is maybe a bit too far integrated into laravel. Do you understand what I try to say here?

In the case we want to keep/add this into this repository the worker shouldn't be the default and therefore moved down the readme as an nice additional feature.

If my code review comments are unclear don't hesitate to ask. Every comment is also discussable, so if you have a strong opinion share it with us.

@bschmitt So should we add the worker as an additional feature or should we cherry pick and outsource this part in a different PR? I'm not sure to be honest. I could live with both options.

@gitkv again. Good job, lets get this stuff in here. :+1:

weblogics commented 5 years ago

@gitkv Any updates on this? We are looking to implement this and this is the only thing missing?

stevenklar commented 4 years ago

@weblogics you could fork it, resolve the conflicts and I can review, test and merge it. Long awaiting feature. :-)

Or @gitkv finds the time himself.

Don't have time implementing it at the moment.