(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?

(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 ;)

(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!

It seems that Koha/MetaSearcher.pm missed the train.

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