Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Messenger] support for multiple bindings on the same queue#37722
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedAug 1, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks for the PR. For future PRs of yours, please note that we prefer proper description + title on PRs, because that's what is recorded in the changelog and in the git history. Referring to "the linked issue" misses all these benefits. Could you please update the description in such a way that documenting the PR becomes "trivial", as in "almost a copy/paste of the PR's description"? Could you please add some tests also? |
@nicolas-grekas Thanks for your response. Please see the added unit tests and the updated description. I am not sure I handled deprecation correctly here so feel free to offer any other suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
On a high level, this makes sense. If we allow multiple bindings, then it makes sense for each binding to be able to have its own binding arguments.
| foreach ($queueConfig['bindings'] ?? [] as $binding) { | ||
| $this->queue($queueName)->bind($this->exchangeOptions['name'], $binding['key'] ?? null, $binding['arguments'] ?? []); | ||
| } | ||
| if (isset($queueConfig['bindings']) && empty($queueConfig['binding_keys'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this isn't needed? Because if$queueConfig['binding_keys']) is empty, then the next foreach wouldn't do anything anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The '?? [null]' in the next line will ensure that bind is called at least once with a null $bindingKey if 'binding_keys' is empty. This would have the effect of binding all messages from the exchange to the queue.
| @@ -231,6 +241,8 @@ private static function validateOptions(array $options): void | |||
| if (0 < \count($invalidQueueOptions = array_diff(array_keys($queue), self::AVAILABLE_QUEUE_OPTIONS))) { | |||
| trigger_deprecation('symfony/messenger', '5.1', 'Invalid queue option(s) "%s" passed to the AMQP Messenger transport. Passing invalid queue options is deprecated.', implode('", "', $invalidQueueOptions)); | |||
| } elseif (0 < \count($invalidQueueOptions = array_diff(array_keys($queue), self::AVAILABLE_QUEUE_OPTIONS_52))) { | |||
| trigger_deprecation('symfony/messenger', '5.2', 'Deprecated queue option(s) "%s" passed to the AMQP Messenger transport. The "bindings" option should be used rather than "binding_keys" and "binding_arguments".', implode('", "', $invalidQueueOptions)); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure this is quite right. In 5.1, we ADDED validation of the options in general. In 5.2, you're deprecatingbindings. So basically, if you passbindings, we should say thatbindings is deprecated in 5.2. If we pass any other invalid option, it would use the existing deprecation message.
Also, could we add validation thatbindings only containskey andarguments keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I fixed this and added validation on each individual binding. Thanks!
| @@ -443,6 +455,12 @@ private function setupExchangeAndQueues(): void | |||
| foreach ($this->queuesOptions as $queueName => $queueConfig) { | |||
| $this->queue($queueName)->declareQueue(); | |||
| foreach ($queueConfig['bindings'] ?? [] as $binding) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In the example in the PR description, you show using an array key underbindings - likemanual-messages, but that is not used here. What is the purpose of this YAML key? You also show an example without one. Which is the correct usage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am not sure there is or should be a correct usage. The original intent was that these bindings could have a developer/human friendly name. I have found that using these names for the bindings is useful when we have more than a dozen bindings. In my projects, all of these bindings would ideally have a name. I do not think I would want to force another developer to do this generally though. I'm okay with any suggestions here though.
@weaverryan Can you have another look? |
We've just moved away from |
Uh oh!
There was an error while loading.Please reload this page.
Description
When using header based queues and exchanges to keep track of events, one may want to have multiple bindings on those headers in those queues. Adding a few lines of code to Connection.php in amqp-messenger will allow Symfony Messenger to support this type of queue. This is pretty standard in other amqp library implementations.
Details
This PR introduces a small refactoring that deprecates the binding_keys and binding_arguments options with a new array option, bindings. This bindings options allows any number of arrays with a 'key' string value, another 'arguments' array value, and possibly an 'exchange' value in future revisions to support multiple exchanges.
Example