Bug 21955 - Cache::Memory should not be used as L2 cache
Summary: Cache::Memory should not be used as L2 cache
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-05 18:33 UTC by Jonathan Druart
Modified: 2020-01-06 20:13 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Cache::Memory fails to work correctly under a plack environment as the cache cannot be shared between processes.
Version(s) released in:


Attachments
Bug 21955: Remove Cache::Memory as fallback for L2 cache (3.20 KB, patch)
2018-12-05 19:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21955: Remove Cache::Memory as fallback for L2 cache (3.26 KB, patch)
2018-12-10 11:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21955: Remove Cache::Memory as fallback for L2 cache (3.32 KB, patch)
2018-12-11 08:27 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2018-12-05 18:33:30 UTC
If Plack is running with several workers we must not use Cache::Memory as L2 cache.

If a value is set from a worker, it will not be available from other workers as the Cache::Memory instance is not shared (of course!)

Moreover we now have Koha::Cache::Memory::Lite that does the same job, so we should not expect performance regressions by removing it.
Comment 1 Jonathan Druart 2018-12-05 19:44:16 UTC
Created attachment 82890 [details] [review]
Bug 21955: Remove Cache::Memory as fallback for L2 cache

If Plack is running with several workers we must not use Cache::Memory
as L2 cache.

If a value is set from a worker, it will not be available from other
workers as the Cache::Memory instance is not shared (of course!)

Moreover we now have Koha::Cache::Memory::Lite that does the same job,
so we should not expect performance regressions by removing it.

See also the email sent to koha-devel for more info
http://lists.koha-community.org/pipermail/koha-devel/2018-December/045004.html
Comment 2 Jonathan Druart 2018-12-05 19:45:03 UTC
Maybe the severity can be adjusted later if not considered as critical.
Comment 3 Martin Renvoize 2018-12-10 11:10:14 UTC
Created attachment 83007 [details] [review]
Bug 21955: Remove Cache::Memory as fallback for L2 cache

If Plack is running with several workers we must not use Cache::Memory
as L2 cache.

If a value is set from a worker, it will not be available from other
workers as the Cache::Memory instance is not shared (of course!)

Moreover we now have Koha::Cache::Memory::Lite that does the same job,
so we should not expect performance regressions by removing it.

See also the email sent to koha-devel for more info
http://lists.koha-community.org/pipermail/koha-devel/2018-December/045004.html

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2018-12-10 11:10:37 UTC
I'm fully behind this, signing off.
Comment 5 Julian Maurice 2018-12-10 15:57:48 UTC
I cannot reproduce the issue you mentioned on the ML.

about.pl says

> Effective caching method: Cache::Memory

I have 2 starman workers but reloading the page always show the right value for the syspref.
What am I missing ?
Comment 6 Jonathan Druart 2018-12-10 19:00:05 UTC
(In reply to Julian Maurice from comment #5)
> I cannot reproduce the issue you mentioned on the ML.
> 
> about.pl says
> 
> > Effective caching method: Cache::Memory
> 
> I have 2 starman workers but reloading the page always show the right value
> for the syspref.
> What am I missing ?

Can you confirm you are displaying the pref's value using the Koha.Preference method, i.e. not looking at the pref admin view, which fetches the value from the DB directly?
Also make sure starman's parameter --max-requests is not set to 1 :)
Comment 7 Julian Maurice 2018-12-11 08:27:00 UTC
Ah sorry, I saw 2 starman processes and interpreted that as 2 workers, but it was the master and 1 worker.
If I increase the number of workers, I can reproduce the problem indeed.
Comment 8 Julian Maurice 2018-12-11 08:27:22 UTC
Created attachment 83034 [details] [review]
Bug 21955: Remove Cache::Memory as fallback for L2 cache

If Plack is running with several workers we must not use Cache::Memory
as L2 cache.

If a value is set from a worker, it will not be available from other
workers as the Cache::Memory instance is not shared (of course!)

Moreover we now have Koha::Cache::Memory::Lite that does the same job,
so we should not expect performance regressions by removing it.

See also the email sent to koha-devel for more info
http://lists.koha-community.org/pipermail/koha-devel/2018-December/045004.html

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 9 Nick Clemens 2018-12-11 20:56:47 UTC
Awesome work all!

Pushed to master for 19.05
Comment 10 Martin Renvoize 2018-12-12 10:34:25 UTC
Pushed to 18.11.x for 18.11.01
Comment 11 Jesse Maseto 2018-12-18 19:24:06 UTC
Pushed to 18.05.x for 18.05.07
Comment 12 Fridolin Somers 2018-12-19 13:30:43 UTC
Pushed to 17.11.x for 17.11.13