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

Clean expired quotes - Fix out of memory on huge quotes list #27260

Merged
merged 6 commits into from
Jun 5, 2020
Merged

Clean expired quotes - Fix out of memory on huge quotes list #27260

merged 6 commits into from
Jun 5, 2020

Conversation

ihor-sviziev
Copy link
Contributor

@ihor-sviziev ihor-sviziev commented Mar 12, 2020 •
edited by ghost
Loading

Description (*)

On production we had a lot of expired quotes, and during cron run we were getting fatal error:

PHP Fatal error:  Allowed memory size of 2147483648 bytes exhausted (tried to allocate 20480 bytes) in /path/to/magento2/vendor/magento/framework/Model/AbstractModel.php on line 359

Check https://getcomposer.org/doc/articles/troubleshooting.md#memory-limit-errors for more info on how to handle out of memory errors.

Investigation found that cron job sales_clean_quotes fails with out of memory because it tries to load all quote items at once

Related Pull Requests

N/A

Fixed Issues (if relevant)

N/A

Manual testing scenarios (*)

  1. ...
  2. ...

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] Clean expired quotes - Fix out of memory on huge quotes list #28342: Clean expired quotes - Fix out of memory on huge quotes list

@m2-assistant
Copy link

m2-assistant bot commented Mar 12, 2020

Hi @ihor-sviziev. 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.

@ihor-sviziev ihor-sviziev force-pushed the fix-clean-quotes-failure branch from b9ed75f to 8ea9b0e Compare March 12, 2020 09:44
@ihor-sviziev ihor-sviziev force-pushed the fix-clean-quotes-failure branch from 8ea9b0e to 1031b3b Compare March 12, 2020 10:43
@ihor-sviziev
Copy link
Contributor Author

I think it's a good idea to cover these changes with some test, but have no ideas which test will be ok there

lbajsarowicz
lbajsarowicz reviewed Mar 13, 2020
View reviewed changes
Copy link
Contributor

@lbajsarowicz lbajsarowicz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

If you wait for my pagination fix, your "for" loop with comment would no longer be needed and could be replaced with while.

@ghost ghost assigned lbajsarowicz Mar 13, 2020
@ihor-sviziev
Copy link
Contributor Author

ihor-sviziev commented Mar 13, 2020 via email

@lbajsarowicz
Copy link
Contributor

lbajsarowicz commented Mar 13, 2020 •
edited
Loading

The difference is that with while you continuously get the real amount of entries. With for - you get number of pages once and then iterate

EDIT:

As we discussed on Slack:
Let's assume that we have 3 items per page
We walk through the 1st page removing 1, 2, 3
You switch to 2nd page assuming that you'll remove 4,5,6
However - 4,5,6 is currently 1st page, so you'll remove 7,8,9

So your way leave gaps between the pages.

Solution for that would be while or if you really like for - make it decrementing. getAllIds is also an option.

Also surround single removal with try-catch block - when single removal fails, the script still work.

@ihor-sviziev
Copy link
Contributor Author

Hi @lbajsarowicz,
Could you review my changes again? I covered them with integration test

lbajsarowicz
lbajsarowicz suggested changes Mar 24, 2020
View reviewed changes
Copy link
Contributor

@lbajsarowicz lbajsarowicz left a comment

Choose a reason for hiding this comment

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

Improve readability of Integration Tests, and we're ✔️

dev/tests/integration/testsuite/Magento/Sales/Cron/CleanExpiredQuotesTest.php Outdated Show resolved Hide resolved
dev/tests/integration/testsuite/Magento/Sales/Cron/CleanExpiredQuotesTest.php Outdated Show resolved Hide resolved
lbajsarowicz
lbajsarowicz approved these changes Mar 24, 2020
View reviewed changes
Copy link
Contributor

@lbajsarowicz lbajsarowicz left a comment

Choose a reason for hiding this comment

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

✔️ Great! Thank you for your contribution!

@magento-engcom-team
Copy link
Contributor

Hi @lbajsarowicz, thank you for the review.
ENGCOM-7193 has been created to process this Pull Request
✳️ @lbajsarowicz, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@engcom-Alfa engcom-Alfa self-assigned this Apr 3, 2020
@engcom-Alfa
Copy link
Contributor

@ihor-sviziev in my case, it started failing with 5000 expired quotes and more, with memory_limit=128M

@VladimirZaets VladimirZaets added the Priority: P2 A defect with this priority could have functionality issues which are not to expectations. label Apr 7, 2020
@VladimirZaets VladimirZaets added this to the 2.4.1 milestone Apr 7, 2020
@VladimirZaets VladimirZaets added Priority: P3 May be fixed according to the position in the backlog. Severity: S2 Major restrictions or short-term circumventions are required until a fix is available. and removed Priority: P2 A defect with this priority could have functionality issues which are not to expectations. labels Apr 7, 2020
@ihor-sviziev
Copy link
Contributor Author

@engcom-Alfa for now let's leave it as is, as increasing amount of quotes might cause fatal error in integration tests due to out of memory. Why - because there running also other tests that could leave some not cleaned memory

@VladimirZaets VladimirZaets added Progress: accept QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) and removed Progress: extended testing Progress: accept labels May 20, 2020
@sdzhepa
Copy link
Contributor

sdzhepa commented May 22, 2020

@magento create issue

@ghost ghost mentioned this pull request May 22, 2020
Closed
4 tasks
@sdzhepa sdzhepa linked an issue May 22, 2020 that may be closed by this pull request
[Issue] Clean expired quotes - Fix out of memory on huge quotes list #28342
Closed
4 tasks
@magento-engcom-team magento-engcom-team added QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch and removed QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) labels May 22, 2020
@engcom-Echo
Copy link
Contributor

Failed functional tests not related to the changes in this PR

@ghost ghost added the Progress: accept label May 28, 2020
magento-engcom-team pushed a commit that referenced this pull request Jun 5, 2020
@slavvka
ENGCOM-7193: Clean expired quotes - Fix out of memory on huge quotes …
8ba6b59 
…list #27260
@magento-engcom-team magento-engcom-team merged commit c439a16 into magento:2.4-develop Jun 5, 2020
@m2-assistant
Copy link

m2-assistant bot commented Jun 5, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lbajsarowicz lbajsarowicz lbajsarowicz approved these changes

Assignees

@lbajsarowicz lbajsarowicz

@engcom-Alfa engcom-Alfa

@engcom-Echo engcom-Echo

Labels
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Component: Sales Priority: P3 May be fixed according to the position in the backlog. Progress: accept QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope Release Line: 2.4 Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch Severity: S2 Major restrictions or short-term circumventions are required until a fix is available.
Projects
Pull Requests Dashboard
Archived in project
Milestone
2.4.1
Development

Successfully merging this pull request may close these issues.

[Issue] Clean expired quotes - Fix out of memory on huge quotes list
8 participants
@ihor-sviziev @lbajsarowicz @magento-engcom-team @engcom-Alfa @sdzhepa @engcom-Echo @VladimirZaets @slavvka