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.

Sign up for GitHub

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

Jump to bottom

MysqlMq: Use new MessageQueue Config interface, update unit tests #21942

Closed
wants to merge 1 commit into from
Closed

MysqlMq: Use new MessageQueue Config interface, update unit tests #21942

wants to merge 1 commit into from
+76 −32

Conversation

Berdir
Copy link
Contributor

@Berdir Berdir commented Mar 26, 2019

Description (*)

This fixes the deprecated usage of the old ConfigInterface in MysqlMQ

Fixed Issues (if relevant)

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

Manual testing scenarios (*)

See issue, using the MysqlMq adapter for a message queue is currently not working at all.

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 on Travis CI are green)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Mar 26, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@magento-engcom-team
Copy link
Contributor

Hi @Berdir. 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-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

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

@Berdir
Copy link
Contributor Author

Berdir commented Mar 27, 2019

Had a look at the failing test, but can't reproduce that locally and don't know how that could be related.

I did have a look at the functional tests and realized for example \Magento\MysqlMq\Model\PublisherConsumerTest is completely skipped right now, which is failing hard if you remove that. I started updating that but I don't know enough about Magento and it would be a lot of work. I started updating the config a bit, but then got stuck. Pushed what I have to:https://github.com/Berdir/magento2/tree/functional-mysql-mq-test-updates

I would suggest to consider merging this (assuming the functional test fail was either random or can be fixed) and look into updating the functional tests after that. Right now, the MysqlMq compontent is completely non-functional.

@fooman
Copy link
Contributor

fooman commented Jul 8, 2019

This is the last Travis message

PHP Fatal error:  Uncaught PHPUnit\Framework\Exception: Warning: exec(): Unable to fork [tasklist.exe /fi 'PID eq 8687' /fo CSV /nh 2>&1] in /home/travis/build/magento/magento2/lib/internal/Magento/Framework/Shell.php:58.
/home/travis/build/magento/magento2/dev/tests/integration/framework/bootstrap.php:134
/home/travis/build/magento/magento2/lib/internal/Magento/Framework/Shell.php:58
/home/travis/build/magento/magento2/dev/tests/integration/framework/Magento/TestFramework/Helper/Memory.php:89
/home/travis/build/magento/magento2/dev/tests/integration/framework/Magento/TestFramework/Helper/Memory.php:52
/home/travis/build/magento/magento2/dev/tests/integration/framework/Magento/TestFramework/MemoryLimit.php:127
/home/travis/build/magento/magento2/dev/tests/integration/framework/Magento/TestFramework/MemoryLimit.php:59
/home/travis/build/magento/magento2/dev/tests/integration/framework/Magento/TestFramework/Bootstrap/Memory.php:50
  thrown in /home/travis/build/magento/magento2/dev/tests/integration/framework/bootstrap.php on line 134
Fatal error: Uncaught PHPUnit\Framework\Exception: Warning: exec(): Unable to fork [tasklist.exe /fi 'PID eq 8687' /fo CSV /nh 2>&1] in /home/travis/build/magento/magento2/lib/internal/Magento/Framework/Shell.php:58.
/home/travis/build/magento/magento2/dev/tests/integration/framework/bootstrap.php:134
/home/travis/build/magento/magento2/lib/internal/Magento/Framework/Shell.php:58
/home/travis/build/magento/magento2/dev/tests/integration/framework/Magento/TestFramework/Helper/Memory.php:89
/home/travis/build/magento/magento2/dev/tests/integration/framework/Magento/TestFramework/Helper/Memory.php:52
/home/travis/build/magento/magento2/dev/tests/integration/framework/Magento/TestFramework/MemoryLimit.php:127
/home/travis/build/magento/magento2/dev/tests/integration/framework/Magento/TestFramework/MemoryLimit.php:59
/home/travis/build/magento/magento2/dev/tests/integration/framework/Magento/TestFramework/Bootstrap/Memory.php:50

might be good to re-run the tests on the new testing infrastructure.

@nuzil nuzil self-requested a review July 8, 2019 06:05
@nuzil nuzil self-assigned this Jul 8, 2019
nuzil
nuzil requested changes Jul 13, 2019
View reviewed changes
Copy link
Contributor

@nuzil nuzil left a comment

Choose a reason for hiding this comment

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

Didn't find better way to implement it with current methods available.
But we also can do a new resolver for minimise code duplication and move those 2 repeatable parts of code there.

app/code/Magento/MysqlMq/Model/Driver/Bulk/Exchange.php
$exchanges = $this->messageQueueConfig->getExchanges();
foreach ($exchanges as $exchange) {
// @todo Is there a more reliable way to identify MySQL exchanges?
if ($exchange->getConnection() == 'db') {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use this resolver:
Magento\MysqlMq\Model\ConnectionTypeResolver which will allow you to detect Mysql connection type: getConnectionType. Which is basically if NOT a null, then a Mysql connection

app/code/Magento/MysqlMq/Model/Driver/Exchange.php
$exchanges = $this->messageQueueConfig->getExchanges();
foreach ($exchanges as $exchange) {
// @todo Is there a more reliable way to identify MySQL exchanges?
if ($exchange->getConnection() == 'db') {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use this resolver:
Magento\MysqlMq\Model\ConnectionTypeResolver which will allow you to detect Mysql connection type: getConnectionType. Which is basically if NOT a null, then a Mysql connection

@sidolov
Copy link
Contributor

sidolov commented Jul 29, 2019

@Berdir , I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

@sidolov sidolov closed this Jul 29, 2019
@m2-assistant
Copy link

m2-assistant bot commented Jul 29, 2019

Hi @Berdir, 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.

This was referenced Oct 21, 2019
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nuzil nuzil nuzil requested changes

Assignees

@nuzil nuzil

Labels
Component: MysqlMq Progress: needs update Release Line: 2.3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Berdir @magento-cicd2 @magento-engcom-team @fooman @sidolov @nuzil @davidverholen