Bug 17189 - Add the ability to define several memcached namespaces
Summary: Add the ability to define several memcached namespaces
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 11921 16579 16758 17261
  Show dependency treegraph
 
Reported: 2016-08-24 11:37 UTC by Jonathan Druart
Modified: 2017-12-07 22:21 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Now we have defined several namespaces for our cache system. Instance sysprefs, koha conf (koha-conf.xml) and unit tests are defined in a separate namespace. This permit to: launch the tests without interfering with other cache values; flush the sysprefs cache without flushing all other values. About update: there are changes on koha.psgi, check your situation.
Version(s) released in:


Attachments
Bug 17189: Add the ability to define several memcached namespaces - tests (1.78 KB, patch)
2016-08-24 11:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17189: Add the ability to define several memcached namespaces - Koha::Caches (9.29 KB, patch)
2016-08-24 11:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17189: Add the ability to define several memcached namespaces - replace existing occurrences (13.49 KB, patch)
2016-08-24 11:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17189: Add the ability to define several memcached namespaces - tests (1.83 KB, patch)
2016-09-07 10:36 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 17189: Add the ability to define several memcached namespaces - Koha::Caches (9.36 KB, patch)
2016-09-07 10:36 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 17189: Add the ability to define several memcached namespaces - replace existing occurrences (13.58 KB, patch)
2016-09-07 10:36 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 17189: Use delete to flush a L1 cache namespace (633 bytes, patch)
2016-09-07 10:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17189: Use delete to flush a L1 cache namespace (692 bytes, patch)
2016-09-07 10:55 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 17189: Replace occurrences of 'use Koha::Cache' (4.40 KB, patch)
2016-09-07 10:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17189: Replace occurrences of 'use Koha::Cache' (5.04 KB, patch)
2016-09-07 10:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17189: Use Koha::Caches instead of Koha::Cache in Koha::MetaSearcher (1.03 KB, patch)
2016-09-07 11:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17189: Replace occurrences of 'use Koha::Cache' (5.13 KB, patch)
2016-09-07 13:50 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 17189: Use Koha::Caches instead of Koha::Cache in Koha::MetaSearcher (1.09 KB, patch)
2016-09-07 13:50 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 17189: Add the ability to define several memcached namespaces - tests (1.94 KB, patch)
2016-09-07 15:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17189: Add the ability to define several memcached namespaces - Koha::Caches (9.45 KB, patch)
2016-09-07 15:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17189: Add the ability to define several memcached namespaces - replace existing occurrences (13.65 KB, patch)
2016-09-07 15:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17189: Use delete to flush a L1 cache namespace (796 bytes, patch)
2016-09-07 15:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17189: Replace occurrences of 'use Koha::Cache' (5.15 KB, patch)
2016-09-07 15:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17189: Use Koha::Caches instead of Koha::Cache in Koha::MetaSearcher (1.14 KB, patch)
2016-09-07 15:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17189 [QA Followup] - Missing $ on $key var for unsafe variant (1.10 KB, patch)
2016-09-08 13:20 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-08-24 11:37:24 UTC
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
Comment 1 Jonathan Druart 2016-08-24 11:39:25 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-08-24 11:39:28 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2016-08-24 11:39:32 UTC Comment hidden (obsolete)
Comment 4 Jacek Ablewicz 2016-09-06 16:10:25 UTC
(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.
Comment 5 Jacek Ablewicz 2016-09-07 06:54:23 UTC
(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.
Comment 6 Jacek Ablewicz 2016-09-07 10:24:15 UTC
 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?
Comment 7 Jacek Ablewicz 2016-09-07 10:36:25 UTC Comment hidden (obsolete)
Comment 8 Jacek Ablewicz 2016-09-07 10:36:32 UTC Comment hidden (obsolete)
Comment 9 Jacek Ablewicz 2016-09-07 10:36:42 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2016-09-07 10:45:17 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2016-09-07 10:45:34 UTC
(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!
Comment 12 Jacek Ablewicz 2016-09-07 10:50:03 UTC
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 ;)
Comment 13 Jacek Ablewicz 2016-09-07 10:55:10 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2016-09-07 10:58:29 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2016-09-07 10:58:50 UTC
(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!
Comment 16 Jonathan Druart 2016-09-07 10:59:46 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2016-09-07 11:01:07 UTC
It seems that Koha/MetaSearcher.pm missed the train.
Comment 18 Jonathan Druart 2016-09-07 11:04:12 UTC Comment hidden (obsolete)
Comment 19 Jacek Ablewicz 2016-09-07 13:50:03 UTC Comment hidden (obsolete)
Comment 20 Jacek Ablewicz 2016-09-07 13:50:10 UTC Comment hidden (obsolete)
Comment 21 Tomás Cohen Arazi 2016-09-07 15:58:38 UTC
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
Comment 22 Tomás Cohen Arazi 2016-09-07 15:58:48 UTC
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
Comment 23 Tomás Cohen Arazi 2016-09-07 15:58:54 UTC
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
Comment 24 Tomás Cohen Arazi 2016-09-07 15:59:05 UTC
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
Comment 25 Tomás Cohen Arazi 2016-09-07 15:59:11 UTC
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
Comment 26 Tomás Cohen Arazi 2016-09-07 15:59:18 UTC
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
Comment 27 Kyle M Hall 2016-09-08 11:38:50 UTC
Pushed to master for 16.11, thanks Jonathan!
Comment 28 Kyle M Hall 2016-09-08 13:20:06 UTC
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>
Comment 29 Jonathan Druart 2016-09-08 17:55:12 UTC
Note for RMaints: Do not backport it (at least for now)!
Comment 30 Frédéric Demians 2016-09-08 18:00:48 UTC
(In reply to Jonathan Druart from comment #29)
> Note for RMaints: Do not backport it (at least for now)!

Thanks for this note.
Comment 31 Mason James 2016-11-30 08:16:15 UTC
enhancement, passing for 16.05