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

Feature - ActiveMQ Artemis Support #1

Closed
wants to merge 1 commit into from
Closed

Feature - ActiveMQ Artemis Support #1

wants to merge 1 commit into from

Conversation

atrauzzi
Copy link
Contributor

Hey @makasim! 👋

I've managed to put together a first draft for supporting ActiveMQ Artemis using the Enqueue STOMP driver. I have this working locally and I've done my best to stick to your code style, giving in to the odd improvement.

The biggest change I've made is to plumb the notion of broker extensions a bit further into the hierarchy of objects. I noticed in quite a few places, RabbitMQ preferences are hard-coded because there wasn't any way to check which set of extensions are desired for the connection. I'll make further notes on the code in the PR to help you scan through things.

Let me know what you think and note any changes or improvements you're after. I have not added tests as I am hoping to get help from you on that if it's something you want done.

Closes: php-enqueue/enqueue-dev#1067

if ($this->config['lazy']) {
return new StompContext(function () {
return $this->establishConnection();
}, $useExchangePrefix);
Copy link
Contributor Author

@atrauzzi atrauzzi Sep 22, 2020

Choose a reason for hiding this comment

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

I've pushed $useExchangePrefix into the constructor of StompContext as it's based on the information it gets from the current extension type.

Seemed like a sensible refactor?

Comment on lines +114 to +116
catch (ErrorFrameException $e) {
throw new \Exception($e->getMessage() . "\n" . $e->getFrame()->getBody());
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed while debugging that the error returned by the PHP stomp library can end up hiding deeper errors. I've added this to improve error feedback in those scenarios.

(My specific situation was that I was not using the correct queue name when running my worker, so I kept getting messages about the connection failing, but not the underlying response from the broker saying "that queue doesn't exist". 😅 )

Comment on lines +197 to +198
$artemisHeaders['client-id'] = true ? $this->subscriptionId : null;
$artemisHeaders['durable-subscription-name'] = true ? $subscriptionName : null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've included some artemis specifics here, which leverages the ability to check the extension type configured on the queue.

Comment on lines +79 to +81
if ($this->extensionType === ExtensionType::ARTEMIS) {
return $this->getStompName();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A fix for the initial issue, names in artemis don't have to conform to any specific hierarchy or syntax.

@atrauzzi atrauzzi marked this pull request as ready for review September 22, 2020 15:28
@makasim
Copy link
Member

makasim commented Sep 23, 2020

Thanks for your pull request! We love contributions.

However, this repository is what we call a "subtree split": a read-only copy of one directory of the main enqueue-dev repository. It is used by Composer to allow developers to depend on specific Enqueue package.

If you want to contribute, you should instead open a pull request on the main repository:

https://github.com/php-enqueue/enqueue-dev

Read the contribution guide

https://github.com/php-enqueue/enqueue-dev/blob/master/docs/contribution.md

Thank you for your contribution!

@atrauzzi
Copy link
Contributor Author

Argh, monorepos... Alright, I'll get on that.

@atrauzzi atrauzzi closed this Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't use slashes in and prefix queue name with type in STOMP
2 participants