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

AMPQ Interop migration. #516

Closed
wants to merge 1 commit into from
Closed

Conversation

@makasim
Copy link
Contributor

@makasim makasim commented Jan 16, 2018

The migration to AMQP Interop brings several benefits:

  • The producer can be configured to send a message with delay, TTL, and priority.
  • Consumer supports PsrProcessor. The code in a processor is not coupled to AMQP, so it could be used with any other interop compatible transports. It would be easy to use processors from libraries, like this, or this, or this
  • Stable, semantic OO API.
  • In future it would be possible to use the bundle not only with php-amqplib but bunny or amqp-ext as well. So there is no need in creating same bundle but for amqp ext for example.

The goal is to do migration as backward compatible as possible.

At the end, there wont be a dependency on enqueue, only amqp-interop

@makasim
Copy link
Contributor Author

@makasim makasim commented Jan 19, 2018

@stloyd what do you think of this?

@stloyd stloyd added this to the 2.x.x milestone Jan 19, 2018
@makasim makasim mentioned this pull request Jan 19, 2018
@makasim
Copy link
Contributor Author

@makasim makasim commented Jan 19, 2018

@stloyd could you please create a 2.x branch?

@makasim
Copy link
Contributor Author

@makasim makasim commented Feb 15, 2018

What can I do to proceed with this?

@Evgen14
Copy link

@Evgen14 Evgen14 commented Mar 29, 2018

Is it planned to continue work on the branch?

@makasim
Copy link
Contributor Author

@makasim makasim commented Mar 29, 2018

@Evgen14 I am ready to push this forward, just want to make sure the work is not wasted and would be finally merged.

@Evgen14
Copy link

@Evgen14 Evgen14 commented Mar 29, 2018

@makasim yes, it could be a new version or fork

@makasim
Copy link
Contributor Author

@makasim makasim commented Mar 29, 2018

I don’t want work on a fork

@Evgen14
Copy link

@Evgen14 Evgen14 commented Mar 29, 2018

@makasim can I somehow help to promote this PR?

@makasim
Copy link
Contributor Author

@makasim makasim commented May 6, 2018

Who is in charge of the bundle and making decisions? I am ready to work on this more, just want to be sure it is merged and you maintainers like the idea.

@skafandri
Copy link
Collaborator

@skafandri skafandri commented May 6, 2018

Thank you for the effort. I was checking AMQP interop and it looks promising. I have few comments about DI and tests. I will add them later as I am on mobile now.
Thank you again

@makasim
Copy link
Contributor Author

@makasim makasim commented Aug 1, 2018

@skafandri any news? I'd be happy to work on this (have some time for that now).

@makasim
Copy link
Contributor Author

@makasim makasim commented Aug 2, 2018

@skafandri I tried to keep it as BC as possible. Lots of other things could be improved with breaking changes.

Though I think we should start with as minimin as possible breaking changes and do bigger things later.

{
$ch = $this->getChannel();

// TODO fix it
Copy link

@jderusse jderusse Aug 4, 2018

Should be fixed

// TODO fix it
$context = new \Enqueue\AmqpLib\AmqpContext($this->conn);
$context->setDelayStrategy(new RabbitMqDlxDelayStrategy());
$rp = new \ReflectionProperty($context, 'channel');
Copy link

@jderusse jderusse Aug 4, 2018

Consider avoiding reflection

$rp = new \ReflectionProperty($context, 'channel');
$rp->setAccessible(true);
$rp->setValue($context, $ch);
$rp->setAccessible(false);
Copy link

@jderusse jderusse Aug 4, 2018

Not needed

$queue = $context->createQueue($queueName);
$topic = $context->createTopic($exchangeName);

// TODO I don't know why we cannot bind to the default exchange. Should be investigated.
Copy link

@jderusse jderusse Aug 4, 2018

Should be fixed

@@ -2,6 +2,8 @@

namespace OldSound\RabbitMqBundle\RabbitMq;

use Enqueue\AmqpLib\AmqpContext;
use Interop\Queue\PsrProcessor;
Copy link

@jderusse jderusse Aug 4, 2018

This is not a PSR. The className is confusing

@@ -27,6 +27,54 @@ public function setDeliveryMode($deliveryMode)
return $this;
}

/**
* @return null
Copy link

@jderusse jderusse Aug 4, 2018

What is the point to use a property if the value is always null?

@@ -2,6 +2,11 @@

namespace OldSound\RabbitMqBundle\RabbitMq;

use Enqueue\AmqpTools\RabbitMqDlxDelayStrategy;
use Interop\Amqp\AmqpContext;
Copy link

@jderusse jderusse Aug 4, 2018

Package missing in composer's requirements

Copy link

@jderusse jderusse left a comment

The purpose of this PR is to implement the INTEROP interfaces.

Here you are converting RabbitMqBundle into a wrapper to enqueue which is IMHO not the way to go.

If I would want to use enqueue, I would use it directly.

@makasim
Copy link
Contributor Author

@makasim makasim commented Aug 17, 2018

@jderusse I am trying to keep the migration on to amqp-interop as backward compatible as possible. that's why I added enqueue/amqp-lib as a dependency. In the end, everyone could use any interop compatible transports. Hence, no deps on enqueue, only queue-interop\amqp-interop.

obviously, I am not trying to do a wrapper. I am not going to change the configuration, cli commands, or any public interface in any way. no deps on any enqueue package at the end.

One would like to use bunny or event amqp ext because of their better performance.

@makasim makasim closed this Oct 18, 2018
@makasim makasim mentioned this pull request Oct 22, 2018
@ramunasd ramunasd removed this from the 2.x.x milestone Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants