Add queue interop driver #320

makasim · 2017-07-19T07:49:35Z

The driver can work with any queue interop compatible transport such as:

It also provide an extended driver version for amqp interop compatible transports. It supports some amqp specific features such as queue declaration and message couting.

TODO:

add tests
add doc

I welcome everyone who is interested in MQs to join the queue interop group

acrobat · 2017-07-19T07:57:23Z

Thanks for the PR @makasim!

@sagikazarmark should we still merge this or have new drivers directly in a separate repo like we are going to do in #256? Or still add it here and move the packages out all in one time?

makasim · 2017-07-21T10:39:54Z

Is it going to be merged? I need kind of approval to push it forward (add tests and doc.)

sagikazarmark · 2017-07-21T19:45:52Z

src/Driver/InteropDriver.php

+
+        return $this->consumers[$queueName];
+    }
+}


Missing newline

sagikazarmark · 2017-07-21T19:46:10Z

src/Driver/InteropDriver.php

+use Interop\Queue\PsrConsumer;
+use Interop\Queue\PsrContext;
+
+class InteropDriver implements Driver


Should be final.

sagikazarmark · 2017-07-21T19:47:00Z

I would be more comfortable with a separate repo.

@henrikbjorn can you please grant @acrobat and me access to creating new repos. I would like to start separating drivers in a few weeks.

makasim · 2017-07-24T18:12:13Z

Could you please create a repo for me too so I can move stuff there as well.

kynx · 2017-07-24T18:50:11Z

If someone's going to be separating out drivers can you have a look at https://github.com/bernardphp/OpenCloud? I did a pr for an opencloud driver ages ago- happy to pick it up again if there's any feedback.

henrikbjorn · 2017-07-24T19:01:59Z

I have granted the permissions. man. 24. jul. 2017 kl. 20.50 skrev kynx <[email protected]>:

…

If someone's going to be separating out drivers can you have a look at https://github.com/bernardphp/OpenCloud? I did a pr for an opencloud driver ages ago- happy to pick it up again if there's any feedback. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#320 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABNDSmAlkSg1AkxFvtuf3Oy6qaVbNfRks5sROdkgaJpZM4OcWe0> .

henrikbjorn · 2017-07-25T10:39:56Z

@kynx ill merge the opencloud one and feel free to pick it up again to match the new master,

acrobat · 2017-07-25T11:06:00Z

Yes @kynx we will merge that pr! We will start soon with moving the drivers to separate repo's and then we will also pick up your and this PR!

sagikazarmark · 2017-07-25T15:30:11Z

@henrikbjorn Thanks!

sagikazarmark · 2017-07-25T15:56:51Z

@henrikbjorn can you also please add me to a package on packagist as well?

The vendor is already taken by someone else. You may ask them to add your package and give you maintainership access. If they add you as a maintainer on any package in that vendor namespace, you will then be able to add new packages in that namespace. The packages already in that vendor namespace can be found at bernard

henrikbjorn · 2017-07-26T07:20:03Z

What is your Packagist username?

sagikazarmark · 2017-07-26T07:50:10Z

mark.sagikazar

henrikbjorn · 2017-07-26T07:54:19Z

Done

acrobat · 2017-07-26T07:54:59Z

Thanks @henrikbjorn!

sagikazarmark · 2017-07-26T08:02:33Z

Thanks

makasim · 2017-08-10T12:40:41Z

Added AmqpInteropDriver which extends the functionality of InteropDriver with AMQP specific features. It could be used with php-amqplib, amqp-ext and bunny libs.

makasim · 2017-08-10T12:41:42Z

@sagikazarmark regarding separate repository. Isn't the code still merged to this repository and later it is split to the separate repository. If so, I don't see a reason why should we wait till it happens.

makasim · 2017-08-11T12:24:16Z

Added tests and docs. Ready for review.

sagikazarmark · 2017-08-12T11:49:28Z

Isn't the code still merged to this repository and later it is split to the separate repository.

I am okay with merging code here (with a bit of squashing looking at the history), but I would like to avoid more releases before the driver refactor.

makasim · 2017-08-15T08:47:26Z

@sagikazarmark rebased&squashed

makasim · 2017-08-21T08:39:11Z

@sagikazarmark could you please take a look at this?

acrobat · 2017-08-21T08:44:28Z

I've took some time off and a bit of vacation in the last weeks, but I will make some time to review pr's in the coming days!

makasim · 2017-08-21T08:52:56Z

@acrobat great! just let me know if there is anything that should be done.

sagikazarmark · 2017-08-22T17:57:44Z

TBH I am not 100% sure why there is a separate driver for AMQP. That's the first thing that popped up after a quick review. Can't see any explanation in the docs either.

Other than that it's quite a PR, so I need some more time to review it.

makasim · 2017-08-22T18:18:46Z

@sagikazarmark amqp interop is an exnteded version of queue interop and it defines AMQP specific things such as create\remove a queue, ability to get message count and so on.

Obviously that any amqp interop compatible transport could be used with InteropDriver driver.

The amqp interop currently supports three major amqp clients available in PHP: php-amqplib, amqp-ext and bunny and any of them will work with AmqpInteropDriver.

makasim · 2017-08-22T18:20:10Z

Worth to mention that I am going to provide support for these interop drivers.

sagikazarmark · 2017-08-23T07:01:43Z

Thanks for the explanation, I thought something similar. Just for me to understand: the AmqpInterop driver contains some extra logic for AMQP implementations which are not supported by the plain Interop driver, but any AMQP implementation should work with the normal Interop driver as well.

Is that correct?

I wonder if it would make sense to extend the interop driver instead of passing it to the AmqpInterop driver. From the above explanation it seems it does.

makasim · 2017-08-23T07:08:57Z

Is that correct?

@sagikazarmark you are absolutely right

I wonder if it would make sense to extend the interop driver instead of passing it to the AmqpInterop driver.

we can have one single driver and some if isntanceof AmqpInterop inside to enhance amqp interop features. Let me know if it is more preferred way that the current one.

sagikazarmark · 2017-08-23T11:10:43Z

we can have one single driver and some if isntanceof AmqpInterop inside to enhance amqp interop features. Let me know if it is more preferred way that the current one.

Well, if we can say that it's unlikely that over time we have to fill the Interop driver with tons of ifs to support all kinds of interop variants then that could work for me as well. Would make things simpler for sure.

makasim · 2017-08-23T12:12:58Z

I don't expect anything to be added over time.

makasim · 2017-08-24T08:21:11Z

@sagikazarmark merged everything in InteropDriver.

sagikazarmark

I had one comment, apart from that looks good to me.

sagikazarmark · 2017-08-24T08:42:23Z

src/Driver/InteropDriver.php

+    /**
+     * @return PsrContext
+     */
+    public function getContext()


Drivers are an internal thing to Bernard. Is there any reason to expose the context?

I see. The method has been removed.

makasim · 2017-08-28T07:33:52Z

@sagikazarmark @acrobat a kind remainder.

Sorry If I am insistent on this one, I don't want to put this into the pending tray

sagikazarmark · 2017-08-28T12:46:37Z

No problem, keep pushing it. I will give it a last shot today.

acrobat

Looks good to me!

sagikazarmark · 2017-09-03T17:13:09Z

Thanks @makasim

makasim · 2017-09-03T17:36:22Z

Thank you too. Would you like to join queue interop group? I would really happy to see you there.

acrobat · 2017-09-03T18:01:34Z

👍 thanks @makasim!

sagikazarmark reviewed Jul 21, 2017

View reviewed changes

src/Driver/InteropDriver.php Outdated

return $this->consumers[$queueName];

}

}

Copy link

Contributor

sagikazarmark Jul 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline

sagikazarmark reviewed Jul 21, 2017

View reviewed changes

makasim mentioned this pull request Aug 2, 2017

Proposals queue-interop/queue-interop#4

Open

Add queue interop driver

9e4fbbb

makasim force-pushed the queue-interop branch from 67ca517 to 9e4fbbb Compare August 15, 2017 08:45

Merge amqp interop drivet to queue interop.

2df38fb

sagikazarmark reviewed Aug 24, 2017

View reviewed changes

remove getContext method.

c6c0a39

acrobat approved these changes Sep 3, 2017

View reviewed changes

sagikazarmark merged commit c1e6755 into bernardphp:master Sep 3, 2017

makasim deleted the queue-interop branch September 3, 2017 17:35

Add queue interop driver #320

Add queue interop driver #320

Conversation

makasim commented Jul 19, 2017 • edited Loading

acrobat commented Jul 19, 2017

makasim commented Jul 21, 2017 • edited Loading

sagikazarmark Jul 21, 2017

Choose a reason for hiding this comment

sagikazarmark Jul 21, 2017

Choose a reason for hiding this comment

sagikazarmark commented Jul 21, 2017

makasim commented Jul 24, 2017

kynx commented Jul 24, 2017

henrikbjorn commented Jul 24, 2017 via email

henrikbjorn commented Jul 25, 2017

acrobat commented Jul 25, 2017 • edited Loading

sagikazarmark commented Jul 25, 2017

sagikazarmark commented Jul 25, 2017

henrikbjorn commented Jul 26, 2017

sagikazarmark commented Jul 26, 2017

henrikbjorn commented Jul 26, 2017

acrobat commented Jul 26, 2017

sagikazarmark commented Jul 26, 2017

makasim commented Aug 10, 2017

makasim commented Aug 10, 2017

makasim commented Aug 11, 2017

sagikazarmark commented Aug 12, 2017

makasim commented Aug 15, 2017

makasim commented Aug 21, 2017

acrobat commented Aug 21, 2017

makasim commented Aug 21, 2017 • edited Loading

sagikazarmark commented Aug 22, 2017

makasim commented Aug 22, 2017 • edited Loading

makasim commented Aug 22, 2017 • edited Loading

sagikazarmark commented Aug 23, 2017

makasim commented Aug 23, 2017 • edited Loading

sagikazarmark commented Aug 23, 2017

makasim commented Aug 23, 2017

makasim commented Aug 24, 2017

sagikazarmark left a comment

Choose a reason for hiding this comment

sagikazarmark Aug 24, 2017

Choose a reason for hiding this comment

makasim Aug 24, 2017

Choose a reason for hiding this comment

makasim commented Aug 28, 2017

sagikazarmark commented Aug 28, 2017

acrobat left a comment

Choose a reason for hiding this comment

sagikazarmark commented Sep 3, 2017

makasim commented Sep 3, 2017

acrobat commented Sep 3, 2017

makasim commented Jul 19, 2017 •

edited

Loading

makasim commented Jul 21, 2017 •

edited

Loading

acrobat commented Jul 25, 2017 •

edited

Loading

makasim commented Aug 21, 2017 •

edited

Loading

makasim commented Aug 22, 2017 •

edited

Loading

makasim commented Aug 22, 2017 •

edited

Loading

makasim commented Aug 23, 2017 •

edited

Loading