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

Catch throwables in mview updating #23125

Conversation

QuentinFarizon
Copy link

@QuentinFarizon QuentinFarizon commented Jun 4, 2019

2.2 : #23124
2.3 : #23125

Description

Certain type of runtime exception weren't caught during mview updating, which caused some mview to be stuck in "processing" mode.
Refer to #23054 for more detail about the issue.

This modification is very similar to the one made here : 5927a75

Fixed Issues

#23054

Manual testing scenarios

make an mview updating crash with a Throwable (not an Exception)
process ends, mview is still in 'processing' in database
it is never picked up again

Questions or comments

This does not prevent against all types of crashes (like process out of memory or server stop abruptly, so it is only a step in making indexing crons more resilient

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)

@m2-assistant
Copy link

m2-assistant bot commented Jun 4, 2019

Hi @QuentinFarizonAfrimarket. 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.3-develop instance - deploy vanilla Magento instance

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

throw $exception;
}
}
}

private function executeAction($action, $currentVersionId, $lastVersionId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typehints, please, and declare strict types should also work, I believe.

Copy link
Author

Choose a reason for hiding this comment

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

@orlangur Done

Copy link
Contributor

Choose a reason for hiding this comment

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

@QuentinFarizonAfrimarket please add declare(strict types) on top and if builds will be green - squash into a single commit. If not, don't add such line and just squash.

Copy link
Author

Choose a reason for hiding this comment

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

Done, tests pass with strict type

@ghost ghost assigned orlangur Jun 4, 2019
@QuentinFarizon QuentinFarizon force-pushed the mview_catch_throwables2.3 branch 3 times, most recently from 51a2019 to 13e57f3 Compare June 4, 2019 22:16
Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

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

@QuentinFarizonAfrimarket nice! Please just make a single commit out of it.

@hostep
Copy link
Contributor

hostep commented Jun 5, 2019

I would honestly keep it as 2 separate commits, they do 2 different things (one bug fix, another is cleanup), so it makes sense to keep them separate in my opinion.

@orlangur
Copy link
Contributor

orlangur commented Jun 5, 2019

@hostep ah, ok, I didn't notice bugfix + refactoring are separated, thought that was in first commit and then some adjustments.

@magento-engcom-team
Copy link
Contributor

Hi @orlangur, thank you for the review.
ENGCOM-5210 has been created to process this Pull Request

@magento-engcom-team
Copy link
Contributor

@QuentinFarizonAfrimarket thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@QuentinFarizon
Copy link
Author

Yes it was my intention to separate behaviour and refactoring :)

@QuentinFarizon QuentinFarizon force-pushed the mview_catch_throwables2.3 branch from 13e57f3 to ba4dff7 Compare July 15, 2019 10:46
@engcom-Foxtrot engcom-Foxtrot self-assigned this Jul 18, 2019
@engcom-Foxtrot
Copy link
Contributor

QA passed.

@magento-engcom-team magento-engcom-team merged commit 3d771d9 into magento:2.3-develop Jul 23, 2019
@m2-assistant
Copy link

m2-assistant bot commented Jul 23, 2019

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

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.3 milestone Jul 23, 2019
@sidolov sidolov added the Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests label Sep 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants