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

\Magento\MysqlMq\Model\Driver\Exchange uses deprecated getQueuesByTopic, results in exception #21904

Closed
Berdir opened this issue Mar 22, 2019 · 24 comments

Comments

@Berdir
Copy link
Contributor

@Berdir Berdir commented Mar 22, 2019

Preconditions (*)

  1. Version 2.3.0

Steps to reproduce (*)

  1. Define necessary info in queue/communication xml files using "db" connection
  2. Publish a message through PublisherInterface::publish()

Expected result (*)

  1. Message is stored.

Actual result (*)

  1. Exception: Message queue topic "X" is not configured.

I was able to get it working by replacing the call with this:

    $queueNames = [];
    $exchanges = $this->messageQueueConfig->getExchanges();
    foreach ($exchanges as $exchange) {
        if ($exchange->getConnection() == 'db') {
            foreach ($exchange->getBindings() as $binding) {
                if ($binding->getTopic() == $topic) {
                    $queueNames[] = $binding->getDestination();
                }
            }
        }
    }

https://devdocs.magento.com/guides/v2.2/release-notes/backward-incompatible-changes/ was not that useful, because clearly you can't just loop over all queues, so I tried to go through the exchange, but didn't know how else to get the current exchange than hardcoding the db connection, as that information is not passed to the class.

The topic is also only an exact match, I'm not sure if there's an API or so that supports matching */# wildcards.

I can create a MR if this goes in the right direction.

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

@magento-engcom-team magento-engcom-team commented Mar 22, 2019

Hi @Berdir. Thank you for your report.
To help us process this issue please make sure that you provided the following information:

  • Summary of the issue
  • Information on your environment
  • Steps to reproduce
  • Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento-engcom-team give me 2.3-develop instance - upcoming 2.3.x release

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

@Berdir do you confirm that you was able to reproduce the issue on vanilla Magento instance following steps to reproduce?

  • yes
  • no
@Berdir

This comment has been minimized.

Copy link
Contributor Author

@Berdir Berdir commented Mar 23, 2019

My installation is pretty vanilla, I just have my single additional module that is trying to publish a message, I don't know if you can reproduce this without any custom code.

@Berdir

This comment has been minimized.

Copy link
Contributor Author

@Berdir Berdir commented Mar 24, 2019

\Magento\MysqlMq\Setup\Recurring::install is another class that uses the deprecated service and isn't working anymore.

Here's my fix for that after updating the use statement:

    $queues = [];
    foreach ($this->messageQueueConfig->getQueues() as $queue) {
        $queues[] = $queue->getName();
    }
@engcom-backlog-nazar engcom-backlog-nazar self-assigned this Mar 26, 2019
@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

@magento-engcom-team magento-engcom-team commented Mar 26, 2019

Hi @engcom-backlog-nazar. Thank you for working on this issue.
In order to make sure that issue has enough information and ready for development, please read and check the following instruction: 👇

  • 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).

    DetailsIf the issue has a valid description, the label Issue: Format is valid will be added to the issue automatically. Please, edit issue description if needed, until label Issue: Format is valid appears.

  • 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue. If the report is valid, add Issue: Clear Description label to the issue by yourself.

  • 3. Add Component: XXXXX label(s) to the ticket, indicating the components it may be related to.

  • 4. Verify that the issue is reproducible on 2.3-develop branch

    Details- Add the comment @magento-engcom-team give me 2.3-develop instance to deploy test instance on Magento infrastructure.
    - If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
    - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!

  • 5. Verify that the issue is reproducible on 2.2-develop branch.

    Details- Add the comment @magento-engcom-team give me 2.2-develop instance to deploy test instance on Magento infrastructure.
    - If the issue is reproducible on 2.2-develop branch, please add the label Reproduced on 2.2.x

  • Next steps are available in case you are a member of Community Maintainers.

  • 6. Add label Issue: Confirmed once verification is complete.

  • 7. Make sure that automatic system confirms that report has been added to the backlog.

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

@magento-engcom-team magento-engcom-team commented Mar 26, 2019

Confirmed by @engcom-backlog-nazar
Thank you for verifying the issue. Based on the provided information internal tickets MAGETWO-98869 were created

Issue Available: @engcom-backlog-nazar, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

@Berdir

This comment has been minimized.

Copy link
Contributor Author

@Berdir Berdir commented Mar 26, 2019

I bit confused about all the automated updates, I'm new to this. Got a notifcation about a question how I see it is deprecated, but that seems to be have been deleted again, the whole interface is completely deprecated: \Magento\Framework\MessageQueue\ConfigInterface, it is the API for the 2.2-style events.xml configuration. I'm pretty confused about the deprecation process here.

As I said, happy to provide a merge request, just need feedback on the topic matching, if there's an existing API that would support those wildcards and how that previous worked. Or if it's not supported by the mysql backend.

@engcom-backlog-nazar

This comment has been minimized.

Copy link
Contributor

@engcom-backlog-nazar engcom-backlog-nazar commented Mar 26, 2019

HI @Berdir feel free to provide a PR, i'm just little bit confused and dont see that all class are deprecated. the issue is confirmed.

@Berdir

This comment has been minimized.

Copy link
Contributor Author

@Berdir Berdir commented Mar 26, 2019

Created a merge request, also updated the unit tests, still some open questions about the implementation.

This shows the limits of unit tests perfectly, they still worked just fine. Would be useful to have an actual integration test, but I don't know yet how that works in Magento.

Note that PhpStorm shows the old Interface also as deprecated:

Selection_504

@magento-engcom-team magento-engcom-team added this to Ready for QA in Community Backlog Mar 27, 2019
@m2-backlog m2-backlog bot moved this from Ready for QA to PR In Progress in Community Backlog Mar 27, 2019
@magento-engcom-team magento-engcom-team removed this from PR In Progress in Community Backlog Mar 28, 2019
@magento-engcom-team magento-engcom-team added this to Ready for QA in Community Backlog Mar 28, 2019
@magento-engcom-team magento-engcom-team removed this from Ready for QA in Community Backlog Mar 28, 2019
@magento-engcom-team magento-engcom-team added this to Ready for QA in Community Backlog Mar 28, 2019
@m2-backlog m2-backlog bot moved this from Ready for QA to PR In Progress in Community Backlog Mar 28, 2019
@erfanimani

This comment has been minimized.

Copy link
Contributor

@erfanimani erfanimani commented Jul 8, 2019

Thanks @Berdir — this must have taken a while to debug. I've created a patch from your commit and can confirm it works (2.3.1 and 2.3.2).

@m2-backlog m2-backlog bot unassigned Berdir Jul 29, 2019
@m2-assistant

This comment has been minimized.

Copy link

@m2-assistant m2-assistant bot commented Sep 27, 2019

Hi @Zipmantos! 👋
Thank you for joining. Please accept team invitation 👉 here 👈 and self-assign the issue.

@hostep

This comment has been minimized.

Copy link
Contributor

@hostep hostep commented Sep 27, 2019

@erfanimani:

You can’t have multiple consumer processes running at the same time. The process will create a PID file inside the var directory named queueName.pid. Magento will check this file to see if there already is a consumer process running that’s polling. If there already is a running process, the new command will terminate immediately without consuming anything.

This is just FYI: the check with PID files will change again in the near future (2.3.3 already seems to contain the change) where they now will use another way of locking (probably using the database), is being introduced by 1d9e07b (might be interesting for your blog post)

@erfanimani

This comment has been minimized.

Copy link
Contributor

@erfanimani erfanimani commented Oct 2, 2019

Thanks for the tip @hostep ! I'll add it in.

@Tjitse-E Tjitse-E self-assigned this Oct 21, 2019
@m2-assistant

This comment has been minimized.

Copy link

@m2-assistant m2-assistant bot commented Oct 21, 2019

Hi @Tjitse-E. Thank you for working on this issue.
Looks like this issue is already verified and confirmed. But if you want to validate it one more time, please, go though the following instruction:

  • 1. Add/Edit Component: XXXXX label(s) to the ticket, indicating the components it may be related to.

  • 2. Verify that the issue is reproducible on 2.3-develop branch

    Details- Add the comment @magento give me 2.3-develop instance to deploy test instance on Magento infrastructure.
    - If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
    - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!

  • 3. If the issue is not relevant or is not reproducible any more, feel free to close it.


@m2-backlog m2-backlog bot moved this from Good First Issue to Dev in Progress in Community Backlog Oct 21, 2019
@Tjitse-E

This comment has been minimized.

Copy link
Contributor

@Tjitse-E Tjitse-E commented Oct 21, 2019

@Berdir I can't reproduce this on 2.3-dev. Can you check this again? I've created a sample module that processes queues without any problems (https://github.com/Vendic/magento2-mysql-queue-sample-module/tree/master). Let me know if i've missed something, but for now let's close this issue.

@Tjitse-E Tjitse-E closed this Oct 21, 2019
@m2-backlog m2-backlog bot moved this from Dev in Progress to Done (last 30 days) in Community Backlog Oct 21, 2019
@erfanimani

This comment has been minimized.

Copy link
Contributor

@erfanimani erfanimani commented Oct 21, 2019

@erfanimani

This comment has been minimized.

Copy link
Contributor

@erfanimani erfanimani commented Oct 21, 2019

A couple things:

queue.xml is deprecated. See here https://devdocs.magento.com/guides/v2.3/extension-dev-guide/message-queues/queue-migration.html

The existing queue.xml file is deprecated.

As mentioned, queue.xml is now split into the following three files:

  • queue_consumer.xml - Defines the relationship between an existing queue and its consumer.
  • queue_topology.xml- Defines the message routing rules and declares queues and exchanges.
  • queue_publisher.xml - Defines the exchange where a topic is published.

Your repository @Tjitse-E makes use of both the deprecated queue.xml and the new files, which should be considered unusual. Then I looked at the updated DevDocs here https://devdocs.magento.com/guides/v2.3/extension-dev-guide/message-queues/config-mq.html

And it mentions:

queue.xml - Defines brokers that processes topics. Use for db (MySQL) connections only. Do not create this file for amqp (RabbitMQ) connections.

What the...

What I think happened, somehow, someone mistakenly took this bug as if it was not a bug and wrote in the DevDocs that you need both the deprecated queue.xml file and the other files. So now you're specifying your XML in two different ways. One which is ignored because of the bug, and one that's deprecated but still works. 🤦‍♂

tldr: bad workaround for this bug has made it's way in the DevDocs, so you can't reproduce when following DevDocs. Correct workaround is to patch the bug with related PR and use the new queue xml format anyway.

@Tjitse-E

This comment has been minimized.

Copy link
Contributor

@Tjitse-E Tjitse-E commented Oct 21, 2019

@erfanimani thanks for clarifying. When I remove queue.xml the issue appears.

@Tjitse-E Tjitse-E referenced this issue Oct 21, 2019
3 of 4 tasks complete
Tjitse-E added a commit to Vendic/magento2-mysql-queue-sample-module that referenced this issue Oct 21, 2019
@m2-assistant

This comment has been minimized.

Copy link

@m2-assistant m2-assistant bot commented Oct 21, 2019

Hi @Tjitse-E. Thank you for working on this issue.
In order to make sure that issue has enough information and ready for development, please read and check the following instruction: 👇

  • 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).

    DetailsIf the issue has a valid description, the label Issue: Format is valid will be added to the issue automatically. Please, edit issue description if needed, until label Issue: Format is valid appears.

  • 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue. If the report is valid, add Issue: Clear Description label to the issue by yourself.

  • 3. Add Component: XXXXX label(s) to the ticket, indicating the components it may be related to.

  • 4. Verify that the issue is reproducible on 2.3-develop branch

    Details- Add the comment @magento give me 2.3-develop instance to deploy test instance on Magento infrastructure.
    - If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
    - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!


@m2-backlog m2-backlog bot moved this from Ready for QA to Dev in Progress in Community Backlog Oct 21, 2019
@Tjitse-E Tjitse-E referenced this issue Oct 25, 2019
4 of 4 tasks complete
@erfanimani

This comment has been minimized.

Copy link
Contributor

@erfanimani erfanimani commented Nov 1, 2019

No worries, thanks for checking @Tjitse-E

Seems Magento Core is also doing it that way.. https://github.com/magento/magento2/tree/2.3-develop/app/code/Magento/ImportExport/etc

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

@magento-engcom-team magento-engcom-team commented Nov 12, 2019

@m2-backlog m2-backlog bot moved this from Dev in Progress to Done (last 30 days) in Community Backlog Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Community Backlog
  
Done (last 30 days)
8 participants
You can’t perform that action at this time.