Passing arguments for queues as expected

[bartoszkubicki]

Description (*)

According to docs it should be possible to declare queue with arguments (like dead-letter-exchange or message-ttl). Unfortunately during process of setting up rabbite schema, arguments are lost and queues are declared without any arguments.

It is because here arguements are always empty array. PR fixes this and also adds return type and some minor formatting fixes. If there are any tests for these class I will fix them as soon as bug will be reproduced, confirmed and my solution will be accepted.
If PR will be accepted, please port to 2.3 as well.

Manual testing scenarios (*)

1. Create some schema for rabbit using magento xml queue_topology.xml, like in dev docs.
2. Check if queue is declared in rabbit with arguments (for testing use real arguments like message ttl)

Questions or comments

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds are green)

Resolved issues:

1. resolves [Issue] Passing arguments for queues as expected #30216: Passing arguments for queues as expected

[m2-assistant]

Hi @bartoszkubicki. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[bartoszkubicki]

Static tests seems to fail because of some copy-paste in EE. I have encountered this failure in multiple PR opened.

[ihor-sviziev]

@magento run all tests

[ihor-sviziev]

Hi @bartoszkubicki,
We're really sorry for long waiting for processing this PR. I'm taking this PR and will try to push it forward.

From first looks changes looks good, but need to see the test results and analyze a bit deeper.

[ihor-sviziev]

Unfortunately it might be not an equal to the array_merge. Could you fix it as described here https://github.com/magento/magento-coding-standard/blob/develop/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.md?

[ihor-sviziev]

@magento create issue

[ihor-sviziev]

Hi @bartoszkubicki,
Thank you for your contribution!
Your changes looks good, but small adjustments are needed here. Could you review my comments and update your PR accordingly?

[ihor-sviziev]

Could you add "Mock" suffix to all property names that contains mock object?

[ihor-sviziev]

1. Could you remove the space between type casting and variable in all places in order to have similar code formatting as in other places in Magento, like this?

Suggested change

	(string) $binding['destination'],
	(string)$binding['destination'],

2. Do we really need this casting? Do we have any case where we'll have another type?

[engcom-Foxtrot]

Hello, @bartoszkubicki. Thank you for your contribution.
I've tried to reproduce the issue you've described on default Magento 2 2.4-develop branch without your fix. I've modified app/code/Magento/WebapiAsync/etc/queue_topology.xml to look like:

<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework-message-queue:etc/topology.xsd">
    <exchange name="magento" type="topic" connection="amqp">
        <binding id="async.operations.all" topic="async.#" destinationType="queue" destination="async.operations.all">
            <arguments>
                <argument name="x-message-ttl" xsi:type="number">36000</argument>
            </arguments>
        </binding>
    </exchange>
</config>

After that, I've checked the modified binding through RabbitMQ UI and found that argument has been applied:

Could you please confirm that issue is already fixed or provide some more detailed steps to reproduce?

Pull Request state was updated. Re-review required.

[bartoszkubicki]

@engcom-Foxtrot I will check it. I don't remeber why this branch is set to be merged to 2.4-develop. It was problem for sure in magento 2.3

[bartoszkubicki]

@engcom-Foxtrot Ok, I have checked it against 2.3 and 2.4. I can still reproduce an issue. The thing is, I was expecting to set these arguments on queue level, now I see it may work with this codebase on binding level.

We can see that aguments are set on binding level, but not as a 'feature' of whole queue

Here we have different effect:

Is is achieved by this script:

<?php

declare(strict_types=1);

require_once __DIR__ . '/../vendor/autoload.php';

use PhpAmqpLib\Connection\AMQPStreamConnection;
use PhpAmqpLib\Wire\AMQPTable;

$connection = new AMQPStreamConnection('rabbitmq', 5672, 'user', 'pass', 'vhost');
$channel = $connection->channel();

$taskExchange = 'task';
$taskQueue = 'task';
$deadLetterExchange = 'retry';
$deadLetterQueue = 'retry_task';

// Normal queue
$channel->exchange_declare($taskExchange, 'direct', false, true);
$channel->queue_declare($taskQueue, false, true, false, false, false, new AMQPTable([
    'x-dead-letter-exchange' => $deadLetterExchange
]));
$channel->queue_bind($taskQueue, $taskExchange);

// Retry queue with TTL
$channel->exchange_declare($deadLetterExchange, 'direct', false, true);
$channel->queue_declare($deadLetterQueue, false, true, false, false, false, new AMQPTable([
    'x-dead-letter-exchange' => $taskExchange,
    'x-message-ttl' => 10000
]));
$channel->queue_bind($deadLetterQueue, $deadLetterExchange);
$channel->basic_qos(null, 1, null);

Here difference is also visible:

I think our misunderstanding,in my opinion, is effect of highly inuintuitive queue declaration (#26969). They are normally declared explicitly (like in script above) and with use of Magento MQ Framework, they are "side effect" of declaring bindings. In other words queue don't have to exist when we create binding. If you can explain why all elements are not declared separately (exchange, queue) and later bound by binding I will be grateful.

With fix arguments are applied both to queue and binding, which is also not perfect solution, but it is still somehow fixes the issue.

In order to test it, this PR have to be applied (#26967). It is like this because the same piece of configuration is used or queues and bindings.

From my tests, for now I assume that arguments on bindings level are not equal to bindings on queue level.
Official documentation says (in case of forming dead-letter exchange, which also requires arguments): "To set the dead letter exchange for a queue, specify the optional x-dead-letter-exchange argument when declaring the queue. " - https://www.rabbitmq.com/dlx.html#using-optional-queue-arguments. There is no word about using bindings' arguments for this. I include test zip, where there are simple topologies for dead-letter-exchange feature with different approaches. With queue arguments works and message cycle there and back between queues (max 3 times, because of x-death argument check, but it doesn't really matter, as we test dead letter fallback). With bindings arguments message is dropped after first consumer nack.
Arguments added to binding (as a side effect of fix) don't seem to spoil anything.
test-topology.zip

I think it is still a bug, because if we would like to add some arguments on queue level - it is impossible.

[engcom-Foxtrot]

@bartekszymanski thank you for clarification, but it seems code still works incorrectly.
I'm using config from this PR with some customization:

    <exchange name="test.dead_letter" type="topic" connection="amqp">
        <arguments>
            <argument name="x-dead-letter-exchange" xsi:type="string">test.retry</argument>
        </arguments>
        <binding id="test" topic="#" destinationType="queue"
                 destination="test_dead_letter">
            <arguments>
                <argument name="x-message-ttl" xsi:type="string">36000</argument>
            </arguments>
        </binding>
    </exchange>

As I understand, you assume we should get 'x-dead-letter-exchange' argument set for both topic and queue.
On 2.4-develop branch, I'm getting 'x-dead-letter-exchange' argument set only for the topic.

On bartoszkubicki:message-queue-arguments-passing-from-config branch, the topic is not created at all:

[bartoszkubicki]

@engcom-Foxtrot I can't locate from which place you have taken this config, but it is not about setting arguments for exchanges (it seems to be working), but for queues - so it is not relevant (as official rabbit mq docs says, in order to create working DLX fallback you have to add dlx to queue, not to another exchange, because message is sent to DLX after its expired, rejected and so on).

From my tests on 2.4 you can set arguments for exchanges and bindings only. Arguments for queues are missing. I would like to have possibility to set arguments separately for queues and bindings. Basing on magento queues xmls construction, with this PR, I achieved setting queues arguments (with bindings arguments set, as a side effect). Normally (using php amq lib) you can set arguments for each component separately - exchange, queue, binding.

[engcom-Foxtrot]

@bartoszkubicki could you please provide some xml what would obviously confirm your PR is fixing the issue you described?

[bartoszkubicki]

@engcom-Foxtrot Me and my agency has created sample module that needs this fix - you can install it and test it or take configuration files. Without fix DLX exchanges are not configured properly.

[ihor-sviziev]

@engcom-Foxtrot could you review #26966 (comment) ?

[engcom-Foxtrot]

@bartoszkubicki thank you for clarification.

[engcom-Foxtrot]

QA passed.

Steps:

1. Add

    <exchange name="test.dead_letter" type="topic" connection="amqp">
        <arguments>
            <argument name="x-dead-letter-exchange" xsi:type="string">test.retry</argument>
        </arguments>
        <binding id="test" topic="#" destinationType="queue"
                 destination="test_dead_letter">
            <arguments>
                <argument name="x-message-ttl" xsi:type="string">36000</argument>
            </arguments>
        </binding>
    </exchange>

to any queue_topology.xml file.
2. Run bin/magento setup:upgrade
3. Check if 'test_dead_letter' queue has 'x-message-ttl' feature set (you can use rabbitmq_management plugin for RabbitMQ).

[m2-assistant]

Hi @bartoszkubicki, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.