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

GH-26255: Refactor CacheInvalidate sendPurgeRequest to fix incorrect tag splitting #26256

Merged
merged 13 commits into from
Aug 13, 2020
Merged

GH-26255: Refactor CacheInvalidate sendPurgeRequest to fix incorrect tag splitting #26256

merged 13 commits into from
Aug 13, 2020

Conversation

moloughlin
Copy link

@moloughlin moloughlin commented Jan 3, 2020 •
edited by ihor-sviziev
Loading

Description (*)

Issue: #26255

  • Refactored sendPurgeRequest to take in an array of tags instead of requiring the caller to implode(). This allows us to do smarter chunking, and avoids the incorrect splitting noted in the linked issue.
  • Backwards compatibility maintained for callers passing in a string, calls from core code have been updated to use the new array form
  • Allowed header size to be passed in via DI

Fixed Issues (if relevant)

  1. Fixes Magento_CacheInvalidate mis-handles very large tag patterns when doing a PURGE #26255
  2. Fixes Varnish "Connection reset by peer" error when large catalog is reindexed on schedule #8815 : Varnish "Connection reset by peer" error when large catalog is reindexed on schedule

Manual testing scenarios (*)

  1. See Magento_CacheInvalidate mis-handles very large tag patterns when doing a PURGE #26255

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)

magentoGH-26255: Refactor CacheInvalidate sendPurgeRequest to allow c…
e212301 
…orrect purge batching, regardless of

the size and content of provided tag pattern.
@moloughlin moloughlin requested a review from kokoc as a code owner January 3, 2020 23:14
@m2-assistant
Copy link

m2-assistant bot commented Jan 3, 2020

Hi @moloughlin. 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: Aligent Consulting partners-contribution Pull Request is created by Magento Partner labels Jan 3, 2020
@moloughlin moloughlin mentioned this pull request Jan 3, 2020
Closed
lbajsarowicz
lbajsarowicz previously requested changes Jan 6, 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.

Could you provide some Unit Tests for that changes?

app/code/Magento/CacheInvalidate/Model/PurgeCache.php Outdated Show resolved Hide resolved
app/code/Magento/CacheInvalidate/Model/PurgeCache.php Outdated Show resolved Hide resolved
@ghost ghost assigned lbajsarowicz Jan 6, 2020
@moloughlin
Copy link
Author

@lbajsarowicz, I've added a test for the splitting of large tag arrays and fixed the other outstanding suggestions. Ready for another look when you are available - thanks!

@moloughlin moloughlin dismissed lbajsarowicz’s stale review January 23, 2020 00:27

Added test coverage as requested.

@moloughlin
Copy link
Author

@lbajsarowicz can you please confirm next steps on this? Thanks.

@lbajsarowicz
Copy link
Contributor

@magento run all tests

@lbajsarowicz
Copy link
Contributor

@moloughlin Let's wait for tests to pass.
Code looks fine, so I'll approve the PR after passing the tests.

Could you eventually merge latest 2.4-develop to your working branch?

@moloughlin
Copy link
Author

@lbajsarowicz awesome, thanks. 2.4-develop merged back in and only the typical false negatives on the test runs 👍

@moloughlin
Copy link
Author

Bump (merged 2.4-develop again since it's been a few weeks...)

@lbajsarowicz
Copy link
Contributor

@moloughlin Thanks for your contribution (again).
We faced false-negative with your PR Static Tests.

After tests pass - I'm going to approve the PR.

@magento-engcom-team
Copy link
Contributor

Hi @lbajsarowicz, thank you for the review.
ENGCOM-7073 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-Kilo
Copy link
Contributor

Failed Functional Tests B2B is not related to changes in current Pull Request.

@engcom-Kilo engcom-Kilo moved this from Testing in Progress to Extended Testing (optional) in Pull Requests Dashboard Jul 9, 2020
@engcom-Kilo engcom-Kilo added the QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) label Jul 9, 2020
VladimirZaets
VladimirZaets requested changes Aug 4, 2020
View reviewed changes
Copy link
Contributor

@VladimirZaets VladimirZaets left a comment

Choose a reason for hiding this comment

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

Hi, @engcom-Kilo are tested this fix with varnish? If not please retest it.

@ghost ghost moved this from Extended Testing (optional) to Changes Requested in Pull Requests Dashboard Aug 4, 2020
@ghost ghost assigned VladimirZaets Aug 4, 2020
@ghost ghost added Progress: needs update Priority: P2 A defect with this priority could have functionality issues which are not to expectations. and removed Progress: extended testing labels Aug 4, 2020
@engcom-Kilo
Copy link
Contributor

@VladimirZaets
Yes, this was tested with Varnish. Works correctly.

ihor-sviziev
ihor-sviziev approved these changes Aug 7, 2020
View reviewed changes
Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

Approved again in order to have an ability to move back to accept

@ghost ghost moved this from Changes Requested to Ready for Testing in Pull Requests Dashboard Aug 7, 2020
@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-7073 has been created to process this Pull Request

@engcom-Kilo engcom-Kilo moved this from Ready for Testing to Extended Testing (optional) in Pull Requests Dashboard Aug 10, 2020
@engcom-Alfa engcom-Alfa added QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope and removed QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) labels Aug 11, 2020
@engcom-Kilo engcom-Kilo moved this from Extended Testing (optional) to Merge in Progress in Pull Requests Dashboard Aug 11, 2020
@magento-engcom-team magento-engcom-team mentioned this pull request Aug 13, 2020
Closed
magento-engcom-team pushed a commit that referenced this pull request Aug 13, 2020
@slavvka
ENGCOM-7073: GH-26255: Refactor CacheInvalidate sendPurgeRequest to f…
2d548ac 
…ix incorrect tag splitting #26256
@magento-engcom-team magento-engcom-team merged commit 22c0827 into magento:2.4-develop Aug 13, 2020
@m2-assistant
Copy link

m2-assistant bot commented Aug 13, 2020

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

@ghost ghost moved this from Merge in Progress to Recently Merged in Pull Requests Dashboard Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VladimirZaets VladimirZaets VladimirZaets requested changes

@lbajsarowicz lbajsarowicz lbajsarowicz approved these changes

@ihor-sviziev ihor-sviziev ihor-sviziev approved these changes

@kokoc kokoc Awaiting requested review from kokoc

Assignees

@lbajsarowicz lbajsarowicz

@ihor-sviziev ihor-sviziev

@VladimirZaets VladimirZaets

@engcom-Kilo engcom-Kilo

Labels
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: bug fix Award: test coverage Component: CacheInvalidate Partner: Aligent Consulting partners-contribution Pull Request is created by Magento Partner Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: accept QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope Release Line: 2.4 Severity: S1 Affects critical data or functionality and forces users to employ a workaround.
Projects
Pull Requests Dashboard
  
Recently Merged
Milestone
No milestone
8 participants
@moloughlin @lbajsarowicz @magento-engcom-team @bradleybrecher @engcom-Kilo @ihor-sviziev @VladimirZaets @engcom-Alfa