Add AMQP interop based driver. #158

makasim · 2017-10-25T13:54:40Z

Advantages:

It would work with any amqp interop compatible transports, such as
- enqueue/amqp-ext based on php amqp extension
- enqueue/amqp-lib based on php-amqplib/php-amqplib
- enqueue/amqp-bunny based on bunny
Supports priorities
Supports delays
Supports ttr
Supports attempts
Contains new options like: vhost, connection_timeout, qos_prefetch_count and so on.
Supports Secure (SSL) AMQP connections.
Add ability to set DSN like: amqp:, amqps: or amqp://user:pass@localhost:1000/vhost

makasim · 2017-10-25T14:04:14Z

I'll work on tests and other stuff once we agree in general on this matter.

makasim · 2017-10-26T08:42:49Z

@samdark @zhuravljov could you please have a look?

samdark · 2017-10-26T08:44:01Z

I don't have enough experience do decide on this one in general so @zhuravljov this one is yours. I'll check code in general.

samdark · 2017-10-26T08:44:38Z

src/drivers/amqp/Queue.php

+
+        $this->context->createProducer()
+
+            // TODO check values compatibility


I'll work on tests and other stuff once we agree in general on this matter.

samdark · 2017-10-26T08:44:57Z

src/drivers/amqp/Queue.php

-        $this->channel->queue_declare($this->queueName, false, true, false, false);
-        $this->channel->exchange_declare($this->exchangeName, 'direct', false, true, false);
-        $this->channel->queue_bind($this->queueName, $this->exchangeName);
+        if ($this->context) return;


Add { and } please.

sure, (that's how it was, I kept original styling).

samdark · 2017-10-26T08:47:39Z

src/drivers/amqp/Queue.php

-        if (!$this->channel) return;
-        $this->channel->close();
-        $this->connection->close();
+        if (!$this->context) return;


Add { and } please.

makasim · 2017-10-26T13:08:01Z

This implementation also supports new features: ttr, attempts, delays, priorities.

makasim · 2017-10-30T08:55:17Z

@zhuravljov I was wondering if you have you got time to look into this PR?

thiagotalma · 2017-10-31T21:40:51Z

src/drivers/amqp/Queue.php

+            'host' => $this->host,
+            'port' => $this->port,
+            'user' => $this->user,
+            'pass' => $this->password,


@makasim I just sent #162 to allow the configuration of vhost. You can include in this PR as well, it is a necessary configuration.

zhuravljov · 2017-11-01T06:47:16Z

@makasim thank you for contribution, I will look and test soon.

makasim · 2017-11-01T08:00:57Z

Added:

Contains new options like: vhost, connection_timeout, qos_prefetch_count and so on.
Supports Secure (SSL) AMQP connections.
Add ability to set DSN like: amqp:, amqps: or amqp://user:pass@localhost:1000/vhost

makasim · 2017-11-15T07:23:21Z

Would it be easier for you to review it if I, instead of changing existing class, create new ones?

makasim · 2017-11-19T09:50:10Z

A test fail is not related to my changes.

makasim · 2017-11-29T17:59:23Z

@samdark @zhuravljov @thiagotalma I addressed all review comments.

samdark · 2017-11-29T17:55:39Z

CHANGELOG.md

@@ -4,8 +4,7 @@ Yii2 Queue Extension Change Log
 2.0.2 under development
 -----------------------

- no changes in this release.
-
+- Enh: Add Amqp Interop driver (makasim)


samdark · 2017-11-29T17:55:59Z

docs/guide/README.md

@@ -18,6 +18,7 @@ Queue Drivers
 * [Db](driver-db.md)
 * [Redis](driver-redis.md)
 * [RabbitMQ](driver-amqp.md)
+* [AMQP Interop)](driver-amqp-interop.md)


Please remove extra ).

samdark · 2017-11-29T18:03:24Z

docs/guide/driver-amqp-interop.md

+
+* It would work with any amqp interop compatible transports, such as 
+
+    * [enqueue/amqp-ext](https://github.com/php-enqueue/amqp-ext) based on [php amqp extension](https://github.com/pdezwart/php-amqp)


php → PHP

samdark · 2017-11-29T18:18:27Z

src/drivers/amqp_interop/Command.php

+/**
+ * Manages application amqp-queue.
+ *
+ * @author Maksym Kotliar <kotlyar.maksim@gmail.com>


@since 2.0.2 (or which version is it going to be released it, @zhuravljov ?)

samdark · 2017-11-29T18:19:39Z

src/drivers/amqp_interop/Queue.php

+ */
+class Queue extends CliQueue
+{
+    const H_ATTEMPT = 'yii-attempt';


What is H_? Do we need it?

H stands for Head.

Head of what?

samdark · 2017-11-29T18:26:44Z

src/drivers/amqp_interop/Queue.php

+                $connectionClass = AmqpLibConnectionFactory::class;
+
+                break;
+            case 'ext':


Move to class constants.

samdark · 2017-11-29T18:26:48Z

src/drivers/amqp_interop/Queue.php

+                $connectionClass = AmqpExtConnectionFactory::class;
+
+                break;
+            case 'bunny':


Move to class constants.

samdark · 2017-11-29T18:27:17Z

src/drivers/amqp_interop/Queue.php

+                break;
+
+            default:
+                throw new \LogicException(sprintf('The given driver "%s" is not supported. Supported are "%s"', $this->driver, implode('", "', $this->supportedDrivers)));


Supported are "%s" → Drivers supported are "%s"

samdark · 2017-11-29T18:27:39Z

src/drivers/amqp_interop/Queue.php

+    {
+        $queue = $this->context->createQueue($this->queueName);
+        $queue->addFlag(AmqpQueue::FLAG_DURABLE);
+        $queue->setArguments(['x-max-priority' => 4]);


What is 4? Why 4?

You can set a priority between 0 and 4. Here's the priorities doc

Resource Usage Considerations If priority queues are desired, we recommend using between 1 and 10. Currently using more priorities will consume more resources (Erlang processes).

There is some in-memory and on-disk cost per priority level per queue. There is also an additional CPU cost, especially when consuming, so you may not wish to create huge numbers of levels. The message priority field is defined as an unsigned byte, so in practice priorities should be between 0 and 255.

I add an ability to configure it and set the default value to 10 (as suggested by off doc).

Removed: ~~Added an exception for the case where a developer wants to use a higher value.~~ because of

Messages with a priority which is higher than the queue's maximum are treated as if they were published with the maximum priority.

samdark · 2017-11-29T18:28:35Z

tests/drivers/amqp_interop/QueueTest.php

+
+    public function testRun()
+    {
+        // Not supported


markTestSkipped()?

makasim · 2017-11-29T19:10:57Z

@samdark done

samdark · 2017-11-29T19:13:00Z

src/drivers/amqp_interop/Queue.php

+ */
+class Queue extends CliQueue
+{
+    const H_ATTEMPT = 'yii-attempt';


I think we need to remove H_.

…nto amqp-interop

makasim · 2017-11-29T19:23:52Z

Travis build fail is not related to my changes

a

makasim · 2017-11-30T10:18:16Z

@zhuravljov could you please give a hand with the error on Travis?

This reverts commit d456b75.

makasim · 2017-11-30T14:41:59Z

@zhuravljov tests failed because of this commit d456b75.

While fixing it I realized that it is a good idea to keep setup broker logic and connection one separate.

If you mix them up you won't be able to do anything if setup broker fails. For example, the broker has the same queue but with a different argument for x-max-priorities and hence it complains. You won't able to do anything from the code, like deleting the queue and creating it from scratch.

zhuravljov · 2017-12-05T04:49:07Z

It is merged. Thank you for big work!

makasim · 2017-12-05T08:59:28Z

Party time! 🎉 🎉 🎉

Thank you guys for your patience and thoughtful reviews!

samdark reviewed Oct 26, 2017

View reviewed changes

samdark assigned zhuravljov Oct 26, 2017

makasim mentioned this pull request Oct 26, 2017

Proposals queue-interop/queue-interop#4

Open

thiagotalma reviewed Oct 31, 2017

View reviewed changes

makasim changed the title ~~Amqp interop based driver.~~ Add AMQ interop based driver. Nov 17, 2017

makasim force-pushed the amqp-interop branch 3 times, most recently from e5eea13 to 44570a3 Compare November 17, 2017 14:20

Add Amqp interop based driver.

ba7a8b4

makasim force-pushed the amqp-interop branch from bfe426f to ba7a8b4 Compare November 17, 2017 18:40

makasim mentioned this pull request Nov 29, 2017

AMQP message should always be acknowledged or rejected #175

Closed

makasim added 4 commits November 29, 2017 14:16

review fixes.

1ae568e

address comments.

e8374f0

add advantages

5739f4d

fix typo

f7c2a19

samdark reviewed Nov 29, 2017

View reviewed changes

address review comments.

a2d3c9d

Fixed changelog formatting [skip ci]

1a66da3

samdark reviewed Nov 29, 2017

View reviewed changes

makasim added 2 commits November 29, 2017 21:14

remove H_

f248f73

Merge branch 'amqp-interop' of github.com:formapro-forks/yii2-queue i…

7040946

…nto amqp-interop

samdark approved these changes Nov 29, 2017

View reviewed changes

makasim added 3 commits November 30, 2017 11:15

[amqp-interop] Fixes long sending of a message

d456b75

re-add deprecation notes.

8110ee8

a

add upgrade note.

9cc7a49

makasim changed the title ~~Add AMQ interop based driver.~~ Add AMQP interop based driver. Nov 30, 2017

fix

0c35b9e

makasim added 3 commits November 30, 2017 14:49

Merge remote-tracking branch 'yii/master' into amqp-interop

0534ac4

Revert "[amqp-interop] Fixes long sending of a message"

32d3c76

This reverts commit d456b75.

keep connection logic and setup broker one separatly

0b20354

makasim mentioned this pull request Dec 1, 2017

[doc] yii2-queue amqp driver php-enqueue/enqueue-dev#282

Merged

remove setup command

54e7299

zhuravljov merged commit b4669cc into yiisoft:master Dec 5, 2017

zhuravljov mentioned this pull request Dec 5, 2017

Queue Interop Support #104

Closed

makasim deleted the amqp-interop branch December 5, 2017 08:58

zhuravljov added a commit that referenced this pull request Dec 6, 2017

#158: Added amqp-interop driver to benchmark

af2daa1

zhuravljov added a commit that referenced this pull request Dec 19, 2017

#158: Russian guide about AMQP Interop [skip ci]

ec424f2

zhuravljov added a commit that referenced this pull request Dec 24, 2017

#158: Additional test cases for AMQP Interop driver

5d209a6

zhuravljov added a commit that referenced this pull request Dec 24, 2017

#158: Fixed priority test for AMQP Interop

b2754ec


		$this->context->createProducer()

		// TODO check values compatibility


		* It would work with any amqp interop compatible transports, such as

		* [enqueue/amqp-ext](https://github.com/php-enqueue/amqp-ext) based on [php amqp extension](https://github.com/pdezwart/php-amqp)

Uh oh!

Add AMQP interop based driver. #158

Add AMQP interop based driver. #158

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!