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

Update composer dependency to fix Redis Key Expiery #25488

Conversation

toxix
Copy link
Member

@toxix toxix commented Nov 6, 2019

Description (*)

I experienced a situation on production system with 15 stores and
organic trafic, where the redis cache grows to over 30GB. It looks like
magento is putting a lot of entries into the redis cache without any
expiary. I needed to limit redis memory to prevent the server from
dying, not how it should be. This had some performance influences.

For this issue there is actualy a fix in place which just need to be
pulled in. This commit is do this. With the fix in place the redis
stabalized at arround 3GB. Over all performance is was also increased
with this change.

Referces:
[1] colinmollenhour/Cm_Cache_Backend_Redis@bc63e72
[2] 32058c7
[3] #25487

Fixed Issues (if relevant)

  1. Redis cache grows unilimmited #25487: Redis cache grows unilimmited

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)

I experienced a situation on production system with 15 stores and
organic trafic, where the redis cache grows to over 30GB. It looks like
magento is putting a lot of entries into the redis cache without any
expiary. I needed to limit redis memory to prevent the server from
dying, not how it should be. This had some performance influences.

For this issue there is actualy a fix in place which just need to be
pulled in. This commit is do this. With the fix in place the redis
stabalized at arround 3GB. Over all performance is was also increased
with this change.

Referces:
[1] colinmollenhour/Cm_Cache_Backend_Redis@bc63e72
[2] magento@32058c7
[3] magento#25487
@toxix toxix requested a review from buskamuza as a code owner November 6, 2019 11:26
@m2-assistant
Copy link

m2-assistant bot commented Nov 6, 2019

Hi @toxix. 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 Guide documentation.

@ihor-sviziev ihor-sviziev self-assigned this Nov 6, 2019
@ihor-sviziev ihor-sviziev added Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Component: Composer labels Nov 7, 2019
@toxix
Copy link
Member Author

toxix commented Nov 10, 2019

Hey @buskamuza ,

Can you let me know when do you plan to do the code review? So I can resolve the conflicting composer.lock at that time.

happy coding
Toxix

@ihor-sviziev
Copy link
Contributor

Hi @toxix,
It's not mandatory to have feedback from @buskamuza.
Also conflict in composer.lock could be fixed just during merging this PR by internal team.
So no action items for now from your side.

@engcom-Alfa
Copy link
Contributor

Hi, @toxix Could you resolve merge conflicts?
Thanks!

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.

@ihor-sviziev
Copy link
Contributor

Hi @engcom-Alfa,
There is conflict only in "content-hash" in composer.lock. It will appear again and again in case if someone in another PR will change this file.

Could you before testing merge it with accepting any part of "content-hash", and then run composer update --lock?

I think it's not worth asking author again and again

@ihor-sviziev
Copy link
Contributor

@engcom-Alfa looks like you didn't ran composer update --lock as it should update the content-hash in composer.lock, or you didn't pushed your changes

@VladimirZaets
Copy link
Contributor

Please resolve conflicts and check that all tests are passing

@ihor-sviziev
Copy link
Contributor

Please resolve conflicts and check that all tests are passing

@VladimirZaets please see #25488 (comment)
This issue will appear again and again until this PR will be merged

@slavvka
Copy link
Member

slavvka commented Jan 9, 2020

@toxix @ihor-sviziev I agree that this file should be regenerated before delivery but we added additional tests and to make sure they are passing we need to have this conflict resolved once more time.

…redis-prevent-growing

# Conflicts:
#	composer.lock
@engcom-Foxtrot
Copy link
Contributor

@slavvka updated.

@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

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

@engcom-Foxtrot engcom-Foxtrot force-pushed the issue-25487_redis-prevent-growing branch from 4affd6f to 46b3c3d Compare January 14, 2020 12:23
@engcom-Foxtrot engcom-Foxtrot force-pushed the issue-25487_redis-prevent-growing branch from 46b3c3d to 75aa464 Compare January 15, 2020 14:57
@slavvka
Copy link
Member

slavvka commented Jan 15, 2020

@engcom-Foxtrot I think we need to regenerate composer.lock only for changed package with something like composer update colinmollenhour/cache-backend-redis in order to avoid bumping versions in other packages

@m2-assistant
Copy link

m2-assistant bot commented Feb 13, 2020

Hi @toxix, 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
Labels
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Award: bug fix Progress: accept Release Line: 2.4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants