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

Specify connexion for db upgrade #18728

Closed
wants to merge 3 commits into from
Closed

Specify connexion for db upgrade #18728

wants to merge 3 commits into from

Conversation

AurelienLavorel
Copy link
Member

@AurelienLavorel AurelienLavorel commented Oct 21, 2018

Specify connexion for db upgrade

Description (*)

Compatibility with Magento Commerce split databases setup.

Fixed Issues (if relevant)

  1. [update] enhance performance on large catalog #16570 (comment)

Manual testing scenarios (*)

As before

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-engcom-team
Copy link
Contributor

Hi @AurelienLavorel. 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 $VERSION instance - deploy vanilla Magento instance

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

@AurelienLavorel
Copy link
Member Author

Hi @ihor-sviziev
Could you launch the EE test please?
Thank you

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Oct 21, 2018

Hi @engcom-backlog-nazar,
Could you please check if the issue is fixed by this PR?

From my perspective it shouldn't work because it using only one connection, while we two - each for different db connections. Unfortunately I can't check it on EE right now.

@AurelienLavorel
Copy link
Member Author

I tried on a fresh setup (with sales and quote on other db) and I have no errors during upgrade, hope you'll have the same result.

@ihor-sviziev ihor-sviziev self-assigned this Oct 22, 2018
@ihor-sviziev
Copy link
Contributor

Hi @AurelienLavorel,
Nazar checked this PR and unfortunately it doesn't work.
Here is screenshot:
image

I see that issue related to different connections for different DBs, each connection don't have access to another DBs, only to own.
I think proper solution will be getting all order addresses as it was before applying #16570 + load some small batches of data and process them + do not use objects, just manipulate with arrays.

For sure it will be slower that implementation from #16570 for single-db installations, but it will be working solution for multi-db installations

@AurelienLavorel
Copy link
Member Author

I can add a check if Magento_ScalableCheckout, Magento_ScalableInventory, or Magento_ScalableOms is enabled, we use old code version (with Magento collection) instead of the SQL query. Can you accept it?

@ihor-sviziev
Copy link
Contributor

But it will be failing with big amount of quotes / orders data

@rikwillems
Copy link
Contributor

Working with batches and doing some cleanup after each iteration helps a lot. It still works slow though. Also I see a lot of order addresses not being updated. Probably be due to quote cleanup in EE.

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Oct 29, 2018

Hi @AurelienLavorel,

I can add a check if Magento_ScalableCheckout, Magento_ScalableInventory, or Magento_ScalableOms is enabled, we use old code version (with Magento collection) instead of the SQL query. Can you accept it?

I think yes, it will fix this issue and at least people without split DBs will have performance improvement.

Please update your PR accordingly

@AurelienLavorel
Copy link
Member Author

I did that, thank you for your time

@rikwillems
Copy link
Contributor

This seems a poor solution as it still blocks large setups with split database to upgrade. I'm working on a solution too.

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Oct 30, 2018

Hi @AurelienLavorel,
@rikwillems prepared PR #18945 for fixing the same issue, will review both of them, and will decide what's better

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Nov 7, 2018

Hi @AurelienLavorel,
I reviewed PR #18945 (and was re-created as #19098), and it looks better, just contains few issues.

So I'm closing this PR as we have better proposition of fixing the same issue.

Thank you for your time!

if (
$this->moduleManager->isEnabled('Magento_ScalableCheckout') ||
$this->moduleManager->isEnabled('Magento_ScalableInventory') ||
$this->moduleManager->isEnabled('Magento_ScalableOms')
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, actually we cannot refer to Magento Commerce modules from Magento Open Source ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants