swarrot / SwarrotBundle

A symfony bundle for swarrot integration
MIT License
89 stars 59 forks source link

RPC responses have to be publish on (AMQP default) exchange #94

Closed olaurendeau closed 7 years ago

olaurendeau commented 7 years ago

While trying to use the RpcServerProcessor I run into the following issue

capture d ecran 2016-12-29 a 20 19 14

Returning [] instead of $this->getExtras(); in RpcServerProcessorConfigurator fixed the issue

namespace Swarrot\SwarrotBundle\Processor\RPC;

class RpcServerProcessorConfigurator implements ProcessorConfiguratorInterface
{
    public function resolveOptions(InputInterface $input)
    {
        return [];
    }
}

RabbitMQ example of RPC pattern image

AMQP 0.9.1 RFC define that "To publish to a reply queue a producer sends a message to the default exchange"

capture d ecran 2016-12-29 a 20 35 58 capture d ecran 2016-12-30 a 11 20 04

So RPC exchange should never be defined by configuration, it should always be empty.

odolbeau commented 7 years ago

As said in the doc, "Reply queues are usually temporary, [...]". How did you configure this processor?

I think this should do the job:

- configurator: swarrot.processor.rpc_server
   extras:
       rpc_exchange: ''

This way you should publish in the default exchange IMO. If it works, maybe it's a good idea to update the processor configurator in order to use en empty exchange by default?

olaurendeau commented 7 years ago

Hello Olivier :)

You're absolutely right :) it would work with this configuration, if configurator was allowing it

- configurator: swarrot.processor.rpc_server
   extras:
       rpc_exchange: ''

and therefore extras.rpc_exchange key became useless, that's why I've proposed to remove it :

      */
      public function getProcessorArguments(array $options)
      {
 -        $exchange = $this->getExtra('rpc_exchange', 'retry');
 -
          return [
              'Swarrot\Processor\RPC\RpcServerProcessor',
 -            $this->factory->getMessagePublisher($exchange, $options['connection']),
 +            $this->factory->getMessagePublisher('', $options['connection']),
              $this->logger,
          ];
      }

       */
      public function resolveOptions(InputInterface $input)
      {
 -        return $this->getExtras();
 +        return [];
      }
  }
odolbeau commented 7 years ago

If the exchange MUST be empty, in this case I merge your PR. :) If it's USUALLY empty, in this case I think we should put an empty exchange by default but let the possibility to use a custom one if needed.

olaurendeau commented 7 years ago

Yes it MUST be empty, elsewhere even using a direct exchange it would just produce a message with a routing_key and Client would have to define a binding between exchange and temporary queue. Which is useless :)

odolbeau commented 7 years ago

Even if it makes no sense for your use case, it's still possible to use a routing_key. :) That's why I would like to keep this option available but not mandatory. Is it OK for you?

olaurendeau commented 7 years ago

No problem for me (I've updated the PR). As it took me a while to understand that "Default exchange" feature of AMQP, would you mind if I document it here : https://github.com/swarrot/swarrot/blob/master/src/Swarrot/Processor/RPC/README.md ?

odolbeau commented 7 years ago

PR looks good to me. :) Good idea for the documentation! Should I wait for it before merging or you'll make another PR?

olaurendeau commented 7 years ago

Updated documentation => https://github.com/swarrot/swarrot/pull/144