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

Add AmqpQueryConsumerCallback #22

Open
nikmx opened this issue Jul 19, 2018 · 7 comments
Open

Add AmqpQueryConsumerCallback #22

nikmx opened this issue Jul 19, 2018 · 7 comments
Labels

Comments

@nikmx
Copy link

nikmx commented Jul 19, 2018

Is the query consumer missing or is it rejected due to some technical reasons?
There was a AmqpRpcMessageConsumerCallback included in early versions of this package.

@nikmx
Copy link
Author

nikmx commented Jul 19, 2018

There might also be some inconsistencies concerning query producers.

The producer docs suggest that in the humus config message producers are treated universally. But as the query producer is utilizing the json-rpc-client the humus rpc docs show that configuration would be different.

Additionally as the docs mentioned suggest that producers would be configured universally and resolved via the same factory (e.g. not differing between rpc and regular producers) the factory would need to be sensitive to that, which seems not the case.

@prolic prolic added the bug label Jul 19, 2018
@prolic
Copy link
Member

prolic commented Jul 19, 2018

@nikmx thanks for your comments.

Let's go to the second part first:
You're right, that for the query producer, the factory should check which key it currently reads from and instantiate the query producer instead of the message producer. This is obviously a bug. I mark this issue accordingly. Do you want to provide a PR to improve the factory?

I'm not sure about this AmqpRpcMessageConsumerCallback. Can find it on older releases (v1.1 and v1.0) as well. But maybe I simply forgot what I implemented in the past. If you can link it for me that would be great, so I can tell more about this specifics.
Generally speaking, for RPC queries, I don't think providing a consumer callback that sends messages to the query bus again makes any sense. HumusAmqp is already capable of responding to RPC calls itself, so there is no need for that. See also https://humusamqp.readthedocs.io/en/latest/articles/rpc.html#setup-the-json-rpc-server

@nikmx
Copy link
Author

nikmx commented Jul 19, 2018

@prolic thanks for your reply

I may provide a PR if I resolve my issue utilizing the package. I'll get back to that next week.

Concerning the rpc consumer, find the files attached as text files. A AmqpJsonRpcServer was also included in the package, which now (and maybe back then too) resides in the HumusAmqp package. The consumer I'm thinking of would of course build up on that, but integrating back into the service-bus context. Like directional bridging buses. Like what that package provides for commands and events. I was also reflecting if the communication pattern is valid for queries, but the dedicated callback queues of rpc calls (like rabbitmq integrates) seem reasonable.

AmqpRpcMessageConsumerCallback.php.txt
AmqpJsonRpcServer.php.txt

@prolic
Copy link
Member

prolic commented Jul 19, 2018

I mean if you want to push the queries to the query bus again (which doesn't make any sense imho), then you can do this with the rpc server implementation of humus-amqp. To quote a part from the readme:

function (\Humus\Amqp\JsonRpc\Request $request): \Humus\Amqp\JsonRpc\Response {
    switch ($request->method()) {
        case 'times2':
            return \Humus\Amqp\JsonRpc\JsonRpcResponse::withResult($request->id(), $request->params() * 2);
        case 'times3:
            return \Humus\Amqp\JsonRpc\JsonRpcResponse::withResult($request->id(), $request->params() * 3);
        case 'plus5':
            return \Humus\Amqp\JsonRpc\JsonRpcResponse::withResult($request->id(), $request->params() + 5);
        default:
            return \Humus\Amqp\JsonRpc\JsonRpcResponse::withError($request->id(), new \Humus\Amqp\JsonRpc\JsonRpcError(32601));
    }
}

as you can see routing is already accomplished there, so what's the point of having the query bus additionally?

@nikmx
Copy link
Author

nikmx commented Jul 19, 2018

Hm. If I would have a service bus context here I'd want the messages extracted and passed to the handlers. The routing wouldn't need to be done here, but would integrate in the service-bus implementation and keep everything in service-bus context.

@prolic
Copy link
Member

prolic commented Jul 19, 2018 via email

@nikmx
Copy link
Author

nikmx commented Jul 19, 2018

Ok. Thought it might integrate well, like as bridge for all message types. As the integration was already there, I guess at some point you had that idea too. Thanks for your input.

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

No branches or pull requests

2 participants