OpenMage / magento-lts

Official OpenMage LTS codebase | Migrate easily from Magento Community Edition in minutes! Download the source code for free or contribute to OpenMage LTS | Security vulnerability patches, bug fixes, performance improvements and more.
https://www.openmage.org
Open Software License 3.0
869 stars 436 forks source link

Remove "MAGE" cache tag #1226

Closed joshua-bn closed 1 year ago

joshua-bn commented 4 years ago

Summary (*)

There is a cache tag named "MAGE" which contains every cache item except config. There's no mechanism to actually do anything with this tag.

I have over 2 million cache items on one of my clusters. I tried loading zc:ti:MAGE (or whatever the key is) and it took so long that I was afraid I'd crash my cluster so I stopped.

Examples (*)

        /**
         * Add global magento cache tag to all cached data exclude config cache
         */
        if (!in_array(Mage_Core_Model_Config::CACHE_TAG, $tags)) {
            $tags[] = Mage_Core_Model_App::CACHE_TAG;
        }

Proposed solution

Remove the code above and any other references to Mage_Core_Model_App::CACHE_TAG. Leave the constant there for backwards compatibility.

joshua-bn commented 4 years ago

Discussion from Discord: https://discord.com/channels/408199701196963842/408199701196963844/758049538241069127

colinmollenhour commented 4 years ago

This tag is actually used with the "Flush Magento Cache" button vs the "Flush Cache Storage" button which flushes everything. However, I agree that this is an ill-conceived "feature" because it puts strain on the backend and goes against the whole idea of tagging. I would be for removing this tag and removing the corresponding "Flush Magento Cache" button or changing it's behavior to instead do something like loop through every cache type removing them one at a time. This would also be a good opportunity to implement a safe config refresh inside the UI so that user's don't crash their stores with these buttons.

joshua-bn commented 4 years ago

@colinmollenhour I believe that was the intention, but that's not how it's implemented...

Get the URL https://github.com/OpenMage/magento-lts/blob/1.9.4.x/app/code/core/Mage/Adminhtml/Block/Cache.php#L40

The URL (flushSystem) https://github.com/OpenMage/magento-lts/blob/1.9.4.x/app/code/core/Mage/Adminhtml/Block/Cache.php#L68

Call cleanCache (no tags) https://github.com/OpenMage/magento-lts/blob/1.9.4.x/app/code/core/Mage/Adminhtml/controllers/CacheController.php#L67

Call the clean method of the cache (no tags) https://github.com/OpenMage/magento-lts/blob/1.9.4.x/app/code/core/Mage/Core/Model/App.php#L1227

Jumping through some interfaces and stuff to get to your Redis class: https://github.com/OpenMage/magento-lts/blob/1.9.4.x/lib/Cm/Cache/Backend/Redis.php#L617

It's going to go with the ALL method.

Let me know if I missed something.

colinmollenhour commented 4 years ago

I didn't run a debugger but the method Mage_Core_Model_Cache::clean in the else block calls:

            $res = $this->getFrontend()->clean($mode, array(Mage_Core_Model_App::CACHE_TAG));
            $res = $res && $this->getFrontend()->clean($mode, array(Mage_Core_Model_Config::CACHE_TAG));
joshua-bn commented 4 years ago

Ah yeah, you're right. I missed that one (PhpStorm didn't follow the path, but I put a breakpoint and it hit it). I actually already changed that on my local to $res = $this->getFrontend()->clean(); since that's effectively what it's doing anyway. That should eventually just call Credis_Client::flushDb() now.

Flyingmana commented 4 years ago

as described in the chat, there may be people making use of the cache prefix, and having multiple cache instances in the same redis database, and this cache tag seems to be made to just clean the cache of a certain cache prefix.

But I also think this is a notable difference(and benefit), and it should be possible via an option, or maybe a different module or even cache backend,

joshua-bn commented 4 years ago

Ah, yeah, that makes sense.

Flyingmana commented 4 years ago

so my question is @colinmollenhour @joshua-bn should we add a config option, which omits creation of this cache tag, and also uses flushDb for the clean when active, so users can decide them self in cases, the difference is notable for them?

joshua-bn commented 4 years ago

I guess. The default would have to be enabled or if they have a cache prefix enable it.

colinmollenhour commented 4 years ago

I think we make a decision that fits most users, having too many obscure config options just makes it less likely that users will find and set these options correctly. In this case the only reason to use this tag is if you are sharing the same backend with some other non-magento app which would be terrible practice and I have never seen a good reason to do this.

addison74 commented 1 year ago

@fballiano - I think we should close this issue.