Skip to content

[Backport] #6942 added check for page layout loaded from cache #14129

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
Closed
wants to merge 2 commits into from
Closed

[Backport] #6942 added check for page layout loaded from cache #14129

wants to merge 2 commits into from

Conversation

mrIntegrator
Copy link

@mrIntegrator mrIntegrator commented Mar 16, 2018 •
edited by sidolov
Loading

Description

Under heavy traffic, some pages will become broken because the HTML <head> is not present. The only remedy is to disable layout cache.
When layout updates are loaded from cache, it's assumed that page layout will be loaded from cache as well. The problem is that when Redis reaches its maxmemory setting (as it might under heavy traffic), it will begin evicting keys. So if the page layout key is evicted while the layout key is not, the bug occurs. To fix this bug, we have to check for a successful load of both layout and page layout from the cache.

Fixed Issues (if relevant)

  1. Layout corruption with php-fpm (a Magento 1 problem returns) #6942 (Title is wrong, but issue is real)

Manual testing scenarios

  1. Install Magento with Redis cache (I used db 0 for sessions, 1 for cache, 2 for FPC).
  2. Navigate to a page not cached by FPC (I used customer/account/login).
  3. Delete the page layout cache key in Redis. (Found by doing a var_dump of $cacheIdPageLayout in Magento\Framework\View\Model\Layout\Merge::load).
  4. Refresh the page and behold the headlessness of your broken page.

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)

Original PR

#16428

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Mar 16, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@orlangur orlangur self-assigned this Mar 16, 2018
@mrIntegrator mrIntegrator force-pushed the bugfix/magento2-6942-page-layout-cache-empty branch 2 times, most recently from c8600c8 to 913aacf Compare March 16, 2018 11:24
@orlangur
Copy link
Contributor

Hi @mrIntegrator, any changes should be applied to 2.2-develop first: https://github.com/magento/magento2/blob/2.2-develop/lib/internal/Magento/Framework/View/Model/Layout/Merge.php#L433

Please create a separate PR and sign CLA.

@orlangur orlangur added this to the Release: 2.1.15 milestone Jun 26, 2018
@mrIntegrator mrIntegrator force-pushed the bugfix/magento2-6942-page-layout-cache-empty branch from 913aacf to 75588b6 Compare June 27, 2018 13:04
@mrIntegrator mrIntegrator changed the base branch from 2.1-develop to 2.2-develop June 27, 2018 13:04
@mrIntegrator mrIntegrator force-pushed the bugfix/magento2-6942-page-layout-cache-empty branch from 75588b6 to 8dddc25 Compare June 27, 2018 13:13
@mrIntegrator mrIntegrator changed the base branch from 2.2-develop to 2.1-develop June 27, 2018 13:13
@mrIntegrator mrIntegrator mentioned this pull request Jun 27, 2018
Closed
4 tasks
@mrIntegrator
Copy link
Author

Sorry about the confusion about the base branch. I had misread your comment. The 2.2-develop pull request has been created: #16428. The only difference is in the integration test.

@mrIntegrator mrIntegrator force-pushed the bugfix/magento2-6942-page-layout-cache-empty branch from 8dddc25 to 879383d Compare June 27, 2018 15:21
@orlangur orlangur changed the title #6942 added check for page layout loaded from cache [Backport] #6942 added check for page layout loaded from cache Jul 10, 2018
@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

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

@sidolov
Copy link
Contributor

sidolov commented Aug 2, 2018

Hi @mrIntegrator , please, resolve merge conflict

@mrIntegrator
Copy link
Author

Merge conflict resolved.

@ishakhsuvarov
Copy link
Contributor

Hi @mrIntegrator

Unfortunately, we can not accept this Pull Request to 2.1 release line, as it brings significant performance degradation. You may see attached file for the details.

We are closing this PR now, but you may continue working and improving the same submission for 2.2.

Thank you

metrics-fail-degradation.log.txt

@Swahjak Swahjak mentioned this pull request Mar 25, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orlangur orlangur orlangur approved these changes

Assignees

@orlangur orlangur

Labels
Component: Framework/View Progress: accept Progress: on hold Release Line: 2.1
Projects
None yet
Milestone
Release: 2.1.16
Development

Successfully merging this pull request may close these issues.
6 participants
@mrIntegrator @magento-cicd2 @orlangur @magento-engcom-team @sidolov @ishakhsuvarov