Description
Jonathan Druart
2016-08-24 11:37:24 UTC
Created attachment 54815 [details] [review] Bug 17189: Add the ability to define several memcached namespaces - tests Created attachment 54816 [details] [review] Bug 17189: Add the ability to define several memcached namespaces - Koha::Caches We need to define several namespaces for our cache system. For instance sysprefs, koha conf (koha-conf.xml) and unit tests should be defined in a separate namespace. This will permit to - launch the tests without interfering with other cache values - and flush the sysprefs cache without flushing all other values To do so, we need to store different Koha::Cache objects at a package level. That's why this patch adds a new Koha::Caches module. FIXME: There is an architecture problem here: the L1 cache should be defined in Koha::Cache Created attachment 54817 [details] [review] Bug 17189: Add the ability to define several memcached namespaces - replace existing occurrences s/Koha::Cache->get_instance/Koha::Caches->get_instance (In reply to Jonathan Druart from comment #2) > We need to define several namespaces for our cache system. > For instance sysprefs, koha conf (koha-conf.xml) and unit tests > should be defined in a separate namespace. Hmm, IMO most serious limitation of this approach/implementation is that having separate cache object instances for 1) sysprefs 2) config and 3) everything else will result in opening 3 different connections to (the very same) memcached daemon per script/plack worker. Under plack, the 2nd connection will be used just once. IMO separate object instance (& connection) for sysprefs would have rather limited usability too > This will permit to > - and flush the sysprefs cache without flushing all other values because it will allow to flush only L1 syspref cache selectively, while no such thing would be possible for L2/upstream cache. Unfortunately, with memcached, it's either a flush of the given key or the whole cache flush (all keys in all namespaces & subnamespaces). In memcached, keys from different namespaces are not distinguishable in any way, and deleting keys by wildcard is not possible. (In reply to Jacek Ablewicz from comment #4) > Hmm, IMO most serious limitation of this approach/implementation is that > having separate cache object instances for 1) sysprefs 2) config and 3) > everything else will result in opening 3 different connections to (the very > same) memcached daemon per script/plack worker. Under plack, the 2nd > connection will be used just once. OTOH - those 3 connections to memcached server do not seem to have any significant practical consequences, I did some tests and it's only 150-200 microseconds per connection. In fact it's probably a bit faster then unpatched version (there are 2 separate connections being made in unpatched master) if the Cache::Memcache loading time is taken into account. Memcached is event driven (so the added memory footprint on it's side is negligible, forking not involved) and it is taking the increased burden surprisingly (extremely!) well. sub flush_L1_cache { my( $self ) = @_; - %L1_cache = (); + $L1_cache{$self->{namespace}} = (); } It's setting $L1_cache{'whatever'} to undef, not sure if that's intentional? Probably not an issue, but I haven't done comprehensive tests of this part, it's only really used under plack. Something like %{ $L1_cache{$self->{namespace}} } = (); or delete $L1_cache{$self->{namespace}}; would be more predictable maybe? Created attachment 55284 [details] [review] Bug 17189: Add the ability to define several memcached namespaces - tests Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl> Created attachment 55285 [details] [review] Bug 17189: Add the ability to define several memcached namespaces - Koha::Caches We need to define several namespaces for our cache system. For instance sysprefs, koha conf (koha-conf.xml) and unit tests should be defined in a separate namespace. This will permit to - launch the tests without interfering with other cache values - and flush the sysprefs cache without flushing all other values To do so, we need to store different Koha::Cache objects at a package level. That's why this patch adds a new Koha::Caches module. FIXME: There is an architecture problem here: the L1 cache should be defined in Koha::Cache Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl> Created attachment 55286 [details] [review] Bug 17189: Add the ability to define several memcached namespaces - replace existing occurrences s/Koha::Cache->get_instance/Koha::Caches->get_instance Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl> Created attachment 55291 [details] [review] Bug 17189: Use delete to flush a L1 cache namespace (In reply to Jacek Ablewicz from comment #6) > sub flush_L1_cache { > my( $self ) = @_; > - %L1_cache = (); > + $L1_cache{$self->{namespace}} = (); > } > > It's setting $L1_cache{'whatever'} to undef, not sure if that's intentional? > Probably not an issue, but I haven't done comprehensive tests of this part, > it's only really used under plack. Something like > > %{ $L1_cache{$self->{namespace}} } = (); > > or > > delete $L1_cache{$self->{namespace}}; > > would be more predictable maybe? Definitely, thanks for catching it up! There are many occurrences of 'use Koha::Cache;' left in the code (not replaced by 'use Koha::Caches') - lack of 'use Koha::Caches' shouldn't really be a problem in most places (C4::Context is usually loaded by, well, everything, and it loads Koha::Caches), but some cleanup of those remainings probably wouldn't hurt ;) Created attachment 55292 [details] [review] Bug 17189: Use delete to flush a L1 cache namespace Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl> Created attachment 55293 [details] [review] Bug 17189: Replace occurrences of 'use Koha::Cache' (In reply to Jacek Ablewicz from comment #12) > There are many occurrences of 'use Koha::Cache;' left in the code (not > replaced by 'use Koha::Caches') - lack of 'use Koha::Caches' shouldn't > really be a problem in most places (C4::Context is usually loaded by, well, > everything, and it loads Koha::Caches), but some cleanup of those remainings > probably wouldn't hurt ;) Indeed, done! Created attachment 55294 [details] [review] Bug 17189: Replace occurrences of 'use Koha::Cache' It seems that Koha/MetaSearcher.pm missed the train. Created attachment 55295 [details] [review] Bug 17189: Use Koha::Caches instead of Koha::Cache in Koha::MetaSearcher Created attachment 55301 [details] [review] Bug 17189: Replace occurrences of 'use Koha::Cache' Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl> Created attachment 55302 [details] [review] Bug 17189: Use Koha::Caches instead of Koha::Cache in Koha::MetaSearcher Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl> Created attachment 55316 [details] [review] Bug 17189: Add the ability to define several memcached namespaces - tests Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> https://bugs.koha-community.org/show_bug.cgi?id=11921 Created attachment 55317 [details] [review] Bug 17189: Add the ability to define several memcached namespaces - Koha::Caches We need to define several namespaces for our cache system. For instance sysprefs, koha conf (koha-conf.xml) and unit tests should be defined in a separate namespace. This will permit to - launch the tests without interfering with other cache values - and flush the sysprefs cache without flushing all other values To do so, we need to store different Koha::Cache objects at a package level. That's why this patch adds a new Koha::Caches module. FIXME: There is an architecture problem here: the L1 cache should be defined in Koha::Cache Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> https://bugs.koha-community.org/show_bug.cgi?id=11921 Created attachment 55318 [details] [review] Bug 17189: Add the ability to define several memcached namespaces - replace existing occurrences s/Koha::Cache->get_instance/Koha::Caches->get_instance Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> https://bugs.koha-community.org/show_bug.cgi?id=11921 Created attachment 55319 [details] [review] Bug 17189: Use delete to flush a L1 cache namespace Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> https://bugs.koha-community.org/show_bug.cgi?id=11921 Created attachment 55320 [details] [review] Bug 17189: Replace occurrences of 'use Koha::Cache' Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> https://bugs.koha-community.org/show_bug.cgi?id=11921 Created attachment 55321 [details] [review] Bug 17189: Use Koha::Caches instead of Koha::Cache in Koha::MetaSearcher Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> https://bugs.koha-community.org/show_bug.cgi?id=11921 Pushed to master for 16.11, thanks Jonathan! Created attachment 55341 [details] [review] Bug 17189 [QA Followup] - Missing $ on $key var for unsafe variant Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Note for RMaints: Do not backport it (at least for now)! (In reply to Jonathan Druart from comment #29) > Note for RMaints: Do not backport it (at least for now)! Thanks for this note. enhancement, passing for 16.05 |