[Messenger] Adding final routing key to delay queue name #31355
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
weaverryan
force-pushed
the
fix-delay-queue-routing-key
branch
from
e8f7ba6
to
cb757e2
Compare
Tobion
reviewed
May 6, 2019
src/Symfony/Component/Messenger/Transport/AmqpExt/Connection.php
Outdated
Show resolved
Hide resolved
Tobion
reviewed
May 6, 2019
src/Symfony/Component/Messenger/Tests/Transport/AmqpExt/AmqpExtIntegrationTest.php
Outdated
Show resolved
Hide resolved
weaverryan
force-pushed
the
fix-delay-queue-routing-key
branch
from
cb757e2
to
9940e71
Compare
|
Thank you @weaverryan. |
fabpot
added a commit
that referenced
this pull request
May 7, 2019
…weaverryan) This PR was merged into the 4.3-dev branch. Discussion ---------- [Messenger] Adding final routing key to delay queue name | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #31241 | License | MIT | Doc PR | n/a Fixes #31241. When we delay, we create a queue whose `x-message-ttl` matches the delay length and `x-dead-letter-routing-key` matches the original routing key used for the message. However, before this PR, the original routing key was not part of that queue's name. The result is that if two messages were delayed by the same length, but with different routing keys, the second would try to "redeclare" the existing delay queue with a new `x-dead-letter-routing-key`, resulting in an error similar to: > Server channel error: 406, message: PRECONDITION_FAILED - inequivalent arg 'x-dead-letter-routing-key' for queue 'delay_queue_1000' Integration test was improved to catch this. Cheers! Commits ------- 9940e71 fixing a bug where the delay queue name did not contain the final routing key
Tobion
reviewed
May 7, 2019
| $actualDuration = microtime(true) - $startTime; | ||
|
|
||
| if (\method_exists([$this, 'assertEqualsWithDelta'])) { | ||
| $this->assertEqualsWithDelta($expectedDuration, $actualDuration, 'Duration was not within expected range', .5); |
There was a problem hiding this comment.
the order of arguments is wrong. assertEqualsWithDelta($expected, $actual, float $delta, string $message = '')
weaverryan
added a commit
to weaverryan/symfony
that referenced
this pull request
May 7, 2019
fabpot
added a commit
that referenced
this pull request
May 8, 2019
This PR was merged into the 4.3-dev branch. Discussion ---------- [Messenger] Fixing missed tests from #31355 | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | none | License | MIT | Doc PR | not needed Tests I missed from #31355 - sorry about that! Commits ------- ab37449 Fixing missed tests from #31355
Tobion
added a commit
that referenced
this pull request
Jun 14, 2019
…obion) This PR was merged into the 4.3 branch. Discussion ---------- [Messenger] fix delay delivery for non-fanout exchanges | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> Fixes two bugs and outdated phpdoc: 1. When your exchange is not of type fanout, but direct for example, then delivery of delayed (retrying) messages does not work. This is because the delay logics adds a routing key to message. It was fixed if you have a custom routing key in #31355. But if you have no routing key, it still changed the routing key which means the message will not be delivery from your direct exchange to your queue anymore after being in the delay exchange. For fanout, which is the default, it does not matter because the routing key is ignored. 2. also fix dsn parsing of plain `amqp://` which is a valid URI that parse_url cannot handle when you want to pass all parameters as options Commits ------- 0f15306 [Messenger] fix delay delivery for non-fanout exchanges
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #31241.
When we delay, we create a queue whose
x-message-ttlmatches the delay length andx-dead-letter-routing-keymatches the original routing key used for the message. However, before this PR, the original routing key was not part of that queue's name. The result is that if two messages were delayed by the same length, but with different routing keys, the second would try to "redeclare" the existing delay queue with a newx-dead-letter-routing-key, resulting in an error similar to:Integration test was improved to catch this.
Cheers!