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

Invalidate indexers in RecurringData #26163

Closed

Conversation

fredden
Copy link
Member

@fredden fredden commented Dec 23, 2019

Description

Calling $indexer->reindexAll(); can take a long time with a large database.
This code is run during php bin/magento setup:upgrade, which is part of a
typical deployment.

This change makes the step significantly quicker, while still achieving the
same result. Minimising downtime during deployments seems sensible.

Fixed Issues

Manual testing scenarios

  1. For best results, create a large number (eg 60k) of customers. This step is not required but will more clearly highlight the difference introduced in this change.
  2. Run php bin/magento setup:upgrade
  3. Observe (lack of) delay when running Magento_Customer recurring data setup task.

Questions or comments

I could not see any existing tests covering this code.

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)

Calling `$indexer->reindexAll();` can take a long time with a large database.
This code is run during `php bin/magento setup:upgrade`, which is part of a
typical deployment.

This change makes the step significantly quicker, while still achieving the
same result. Minimising downtime during deployments seems sensible.
@m2-assistant
Copy link

m2-assistant bot commented Dec 23, 2019

Hi @fredden. 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.

@magento-engcom-team magento-engcom-team added Partner: Fisheye partners-contribution Pull Request is created by Magento Partner labels Dec 23, 2019
@rogyar rogyar self-assigned this Dec 23, 2019
@rogyar rogyar added Component: Customer Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests labels Dec 23, 2019
Copy link
Contributor

@rogyar rogyar left a comment

Choose a reason for hiding this comment

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

Hi, @fredden. Thank you for your collaboration. Please, cover your changes with auto-tests. This step is required for further PR processing. I would recommend an integration test that runs the setup:upgrade command and checks the indexer status.

Thank you!

@rogyar
Copy link
Contributor

rogyar commented Dec 23, 2019

Also, according to the failing auto-tests, we may assume that this change brings a couple of other issues as a side-effect.

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Dec 24, 2019

I believe this is related PR #21235 and issue #19469
It wasn’t finished, but had quite long discussion

@fredden fredden requested a review from rogyar January 22, 2020 23:56
@o-iegorov o-iegorov self-assigned this Feb 7, 2020
@ghost ghost unassigned o-iegorov Feb 7, 2020
@ghost
Copy link

ghost commented Feb 7, 2020

@o-iegorov unfortunately, only members of the maintainers team are allowed to assign developers to the pull request

@o-iegorov
Copy link
Contributor

Hi @fredden
I am from internal team and we are started work on this issue in scope of the internal ticket MC-30539

@o-iegorov o-iegorov self-assigned this Feb 7, 2020
@ghost ghost unassigned o-iegorov Feb 7, 2020
@ghost
Copy link

ghost commented Feb 7, 2020

@o-iegorov unfortunately, only members of the maintainers team are allowed to assign developers to the pull request

@o-iegorov o-iegorov self-assigned this Feb 7, 2020
@ghost ghost assigned rogyar and unassigned o-iegorov Feb 7, 2020
@ghost
Copy link

ghost commented Feb 7, 2020

@o-iegorov unfortunately, only members of the maintainers team are allowed to unassign developers from the pull request

@ghost
Copy link

ghost commented Feb 7, 2020

@o-iegorov unfortunately, only members of the maintainers team are allowed to assign developers to the pull request

@ihor-sviziev
Copy link
Contributor

@o-iegorov I've assigned you, but probably you wanted to assign to issue #19469

@fredden
Copy link
Member Author

fredden commented Feb 8, 2020

@o-iegorov are you saying that the work here will be discarded and a different approach is being investigated in private? Should this pull request be closed therefore?

@o-iegorov
Copy link
Contributor

@fredden no, all your commits will be preserved, i plan just to fix failed tests and improve provided integration test

@o-iegorov
Copy link
Contributor

@fredden no need to close this PR

@slackerzz
Copy link
Member

@o-iegorov so the solution to #19469 is to invalidate the index every time we run setup:upgrade?

@hostep
Copy link
Contributor

hostep commented Feb 13, 2020

@slackerzz: I really hope that's not the case, because that would be very bad solution.
I really hope somebody is reading all the comments in #21235, because a lot of research was already done regarding this issue.

@o-iegorov
Copy link
Contributor

@slackerzz in two words yes, your solution absolutely correct: reindex moved to the patch (first time install, need to create indexer tables), invalidation for recurring (upgrade)

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Feb 15, 2020

@o-iegorov,
+1 to @hostep.
Could you explain why marking index as invalid is good idea?, it seems to me as a hack. Why it was done in this way before declarative schema?

@o-iegorov
Copy link
Contributor

@ihor-sviziev for me seems as hack to reindex in upgrade data script (and then moved to recurring script with declarative schema)

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Feb 15, 2020

@o-iegorov Yep, so why we need to have reindex or index invalidation during module upgrades? What is use case why it was added?
As I understood - you’re from internal team, so you should have info about all issue details of commits that introduced this behavior

@o-iegorov
Copy link
Contributor

@ihor-sviziev because no warranty that customer attributes still not changed (by other modules for example) during upgrade

@ihor-sviziev
Copy link
Contributor

@o-iegorov, if some module changed customer attributes - this module should invalidate customer grid index. Am I wrong?
Or maybe it’s better to add some plugin that will invalidate index when customer attribute was added / changed?

@o-iegorov
Copy link
Contributor

@ihor-sviziev yes it's better for sure, but since such behavior was already introduced- in case when this action will be removed (for example from recurring script) some extensions (not magento core) that based on such behavior mail fail after upgrade. To prevent such situation that may treat as backward incompatible changes better to leave already introduced flow as is

@hostep
Copy link
Contributor

hostep commented Feb 15, 2020

I disagree @o-iegorov

The reindexing in the recurring script got added in Magento 2.3 by accident, I will say it was a bug.
It's not normal to expect the bin/magento setup:upgrade command to reindex (or invalidate) an indexer every single time you execute it, that's not expected behavior. I execute that command dozens of times a day, I want it to be fast and efficient, and I really don't want to see it behave the way it does right now.

If people were relying on this incorrect behavior, we just need to make it clear in the release notes of a new Magento version, that they won't be able to rely on this anymore.

It would be nice to get this finally fixed in Magento 2.4.0, as that major version does allow backwards incompatible changes.

@ihor-sviziev
Copy link
Contributor

@o-iegorov i was just writing similar thing while @hostep wrote his comment.
If we’ll add it to release notes - it would be ok. Also next release 2.4.0 - so it’s time to have backward incompatible changes. I think this is really good example to fix the issue in good way.

@o-iegorov
Copy link
Contributor

@hostep even Magento functional test based on such behavior :) Just try to run all function tests on branch with removed app/code/Magento/Customer/Setup/RecurringData.php

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Feb 15, 2020

@o-iegorov tests also depends on update on save index mode, it doesn’t mean that all production websites should work like this. tests cloud be updated if needed, it doesn’t mean it should be in this way.

@o-iegorov
Copy link
Contributor

@ihor-sviziev i'am totally agree that test need to be updated, that was in may comment #26163 (comment)

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Feb 17, 2020

@o-iegorov, but still you want to keep index invalidation? I don't think it's really needed as part of 2.4-develop.

@o-iegorov
Copy link
Contributor

Related internal PR merged, merge commit: cac512f

@o-iegorov o-iegorov closed this Mar 7, 2020
@m2-assistant
Copy link

m2-assistant bot commented Mar 7, 2020

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

@fredden fredden deleted the indexer-setup-upgrade-invalidate branch March 13, 2020 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests Component: Customer Partner: Fisheye partners-contribution Pull Request is created by Magento Partner Progress: needs update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2.3 Customer module Recurring setup script performance problems.
7 participants