Bug 7248 - Caching for services
Summary: Caching for services
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: 3.10
Hardware: All All
: P3 enhancement (vote)
Target Milestone: ---
Assignee: Chris Hall
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 6193 7359
Blocks: 7249 7387 8089
  Show dependency treegraph
 
Reported: 2011-11-20 22:41 UTC by Chris Hall
Modified: 2013-12-05 20:04 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Moved Cache and Memcahed, added syspref and sql entry (10.31 KB, patch)
2011-11-20 22:41 UTC, Chris Hall
Details | Diff | Splinter Review
Git format patch rather than diff (11.15 KB, patch)
2011-11-20 22:46 UTC, Chris Hall
Details | Diff | Splinter Review
Adds in missing sysprefs.sql entry for usecache setting (1.33 KB, patch)
2011-11-21 00:34 UTC, Chris Hall
Details | Diff | Splinter Review
Bug 7248 : Caching support and Syspref (6.23 KB, patch)
2011-11-21 03:05 UTC, Chris Cormack
Details | Diff | Splinter Review
Update to t/Cache.t for better testing of Koha/Cache.pm and Koha/Memcached.pm (2.13 KB, patch)
2011-11-21 20:04 UTC, Chris Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7248 : Caching support and Syspref (6.50 KB, patch)
2011-12-14 07:12 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Unit test for Koha/Cache.pm (which includes Koha/Cache/Memcahed.pm) (2.63 KB, patch)
2011-12-14 07:12 UTC, Katrin Fischer
Details | Diff | Splinter Review
Added caching support and syspref (14.42 KB, patch)
2012-01-27 02:23 UTC, Chris Hall
Details | Diff | Splinter Review
Unit tests for Koha::Cache (incluedes Koha::Cache::Memcached) (2.11 KB, patch)
2012-01-27 02:25 UTC, Chris Hall
Details | Diff | Splinter Review
Bug 7248 Added caching support and syspref and moved Caching into Koha namespace (8.75 KB, patch)
2012-01-28 15:04 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7248 Unit test for Koha/Cache.pm (which includes Koha/Cache/Memcahed.pm) (2.21 KB, patch)
2012-01-28 15:04 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7248 Added caching support and and moved Caching into Koha namespace (6.88 KB, patch)
2012-04-29 13:15 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7248 Unit test for Koha/Cache.pm (which includes Koha/Cache/Memcached.pm) (2.51 KB, patch)
2012-04-29 13:15 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7248 Added caching support and and moved Caching into Koha namespace (6.92 KB, patch)
2012-04-29 19:41 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7248 Unit test for Koha/Cache.pm (which includes Koha/Cache/Memcached.pm) (2.54 KB, patch)
2012-04-29 19:42 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7248 Added caching support and and moved Caching into Koha namespace (8.90 KB, patch)
2012-04-30 01:58 UTC, Mason James
Details | Diff | Splinter Review
Bug 7248/7249 follow-up: more generic cache handling (5.98 KB, patch)
2012-05-14 09:29 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7248 follow-up (2) Adapting subs using Cache to new Caching API (4.89 KB, patch)
2012-05-14 11:27 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7248/7249 follow-up: more generic cache handling (7.56 KB, patch)
2012-05-14 11:28 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7248 follow-up (2) Adapting subs using Cache to new Caching API (4.89 KB, patch)
2012-05-14 11:29 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7248 follow-up (3): cache the cache handler (1.25 KB, patch)
2012-05-14 11:41 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7248 follow-up (alternative) (5.78 KB, patch)
2012-05-14 15:30 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7248 follow-up (alternative) (5.73 KB, patch)
2012-05-14 15:34 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7248 follow-up (alternative) (6.14 KB, patch)
2012-05-14 15:45 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7248 follow-up (alternative) (6.24 KB, patch)
2012-05-14 15:57 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Hall 2011-11-20 22:41:55 UTC Comment hidden (obsolete)
Comment 1 Chris Hall 2011-11-20 22:46:44 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2011-11-20 23:53:09 UTC
Looks good Chris H, only thing missing is the change to 
 installer/data/mysql/sysprefs.sql
Comment 3 Chris Hall 2011-11-21 00:34:57 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2011-11-21 03:05:19 UTC
Have signed off, but leaving it in needs signoff as I think we need a signoff from outside Catalyst.
Comment 5 Chris Cormack 2011-11-21 03:05:42 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2011-11-21 06:09:51 UTC
Hi Chris H., can you please add a description and test plan to this bug? As is I have no idea what it's supposed to do and how to test it. Thx! :)
Comment 7 Chris Hall 2011-11-21 20:04:06 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2011-12-14 07:12:13 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2011-12-14 07:12:21 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2012-01-26 09:05:07 UTC
QA Comment/Questions: 
This patch adds a syspref usecache. I would opt for a more specific name. Note that the word usecache is already used in several javascripts. Something like usememcached for instance?

If such a pref is added, I would also expect to see it being used. But as I understand, the current test is still if $ENV{'MEMCACHED_SERVERS'} is filled in C4/Context. The value of this pref is never tested?

The modules moved to Koha still include references in comments to C4. Could you also eliminate those?
Comment 11 Marcel de Rooy 2012-01-26 09:19:21 UTC
Chris C pointed me to the patches you wrote under 7249 which use the pref. So that question got some clarification.
Although that was the most important one, I still think renaming it would be better. Should not be too hard with an intelligent(!) Replace All ;)
And the C4 mentions ..
Comment 12 Marcel de Rooy 2012-01-26 09:27:47 UTC
Another comment while browsing through 7249:
If we start using your pref, it would be better to eliminate here the old way of testing via $ENV. Make it uniform again.
If you do that, we get rid of a "preference" coming via koha-httpd file. Although it would be nice to seek for some concensus on the list, especially with those who worked on this before..
Comment 13 Katrin Fischer 2012-01-26 10:11:24 UTC
I like the idea of having a system preference to turn it off and on - this will make testing really easy too.
Comment 14 Chris Hall 2012-01-27 02:23:37 UTC Comment hidden (obsolete)
Comment 15 Chris Hall 2012-01-27 02:25:49 UTC Comment hidden (obsolete)
Comment 16 Jared Camins-Esakov 2012-01-28 15:04:27 UTC Comment hidden (obsolete)
Comment 17 Jared Camins-Esakov 2012-01-28 15:04:44 UTC Comment hidden (obsolete)
Comment 18 Paul Poulain 2012-02-01 14:04:59 UTC
I'm uncomfortable with those patches and would like some more explanations
* the memcache information are in httpd.conf, right ? So what could be the use of a "usecache" syspref ? just be easier to switch on/off use of caching ?
*  some things like C4/Languages.pm use memcache as well. Should they rely on this Cache.pm or is it something different ?

(not changing the status, I just want to understand)
Comment 19 Marcel de Rooy 2012-02-01 14:07:20 UTC
(In reply to comment #18)
> I'm uncomfortable with those patches and would like some more explanations
> * the memcache information are in httpd.conf, right ? So what could be the use
> of a "usecache" syspref ? just be easier to switch on/off use of caching ?
> *  some things like C4/Languages.pm use memcache as well. Should they rely on
> this Cache.pm or is it something different ?

Paul, could you also look at comment 12? We would not like to have multiple ways of doing the same.
Comment 20 Katrin Fischer 2012-02-01 14:19:57 UTC
+1 for a system preference to turn off/on memcache.
Comment 21 Paul Poulain 2012-02-01 14:28:19 UTC
(In reply to comment #19)
> Paul, could you also look at comment 12? We would not like to have multiple
> ways of doing the same.

About comment 12 :
If we use syspref to get memcache server informations, it mean we can't cache the syspref (as we need the cache server information to get the cache address)
Isn't this a good reason to keep memcache server in httpd.conf ? what is/are the side effect if any of the $ENV ?
Comment 22 Jared Camins-Esakov 2012-02-01 15:54:37 UTC
(In reply to comment #21)
> (In reply to comment #19)
> > Paul, could you also look at comment 12? We would not like to have multiple
> > ways of doing the same.
> 
> About comment 12 :
> If we use syspref to get memcache server informations, it mean we can't cache
> the syspref (as we need the cache server information to get the cache address)
> Isn't this a good reason to keep memcache server in httpd.conf ? what is/are
> the side effect if any of the $ENV ?

So there's a special case for the UseCache syspref. The side effect of keeping the memcache configuration in $ENV is that we have to explain to non-technical librarians how to adjust their caching settings if they run into problems. This seems like a significant disadvantage, for no real benefit.

Also, regarding the name of the syspref, I think UseCache is better than UseMemcached. One day we may have more than one cache technology available, at which point it could be changed from boolean to a choice, and UseCache = On remapped to UseCache = memcached.
Comment 23 Paul Poulain 2012-02-01 16:28:13 UTC
(In reply to comment #22)
> > Isn't this a good reason to keep memcache server in httpd.conf ? what is/are
> > the side effect if any of the $ENV ?
> 
> So there's a special case for the UseCache syspref. The side effect of keeping
> the memcache configuration in $ENV is that we have to explain to non-technical
> librarians how to adjust their caching settings if they run into problems. This
> seems like a significant disadvantage, for no real benefit.
I think that if a librarian run into problems, the thing that must be done is emptying the cache, that must be done on command line. That's not something for anyone.
Setting the cache server is also something highly technical, and sysprefs should be dedicated to library/user-oriented things.
This setting is the same as for the LDAP = that's in koha.conf, not in syspref.
I think we should have as a rule that :
* technical parameters are in koha.xml or httpd.conf
* librarian parameters are in syspreferences


> Also, regarding the name of the syspref, I think UseCache is better than
> UseMemcached. One day we may have more than one cache technology available, at
> which point it could be changed from boolean to a choice, and UseCache = On
> remapped to UseCache = memcached.
agreed
Comment 24 Jared Camins-Esakov 2012-02-01 16:32:43 UTC
I have no objection to the setting being in koha-conf.xml, if you feel there is no way to specify cache settings in system preferences (I'd argue that LDAP should also be specified in system preferences, and that it's oversight that it still requires editing configuration files manually). koha-conf is a place for *Koha* configuration settings. The Apache config is not.
Comment 25 Paul Poulain 2012-02-02 17:25:37 UTC
I think we must discuss of where to put those files.

I plan to send mails to koha-devel about all "in discussion" bugs, will do that tomorrow
Comment 26 Tomás Cohen Arazi 2012-02-06 19:44:00 UTC
(In reply to comment #24)
> I have no objection to the setting being in koha-conf.xml, if you feel there is
> no way to specify cache settings in system preferences (I'd argue that LDAP
> should also be specified in system preferences, and that it's oversight that it
> still requires editing configuration files manually). koha-conf is a place for
> *Koha* configuration settings. The Apache config is not.

First, I just noticed this bug. I hope it is not late to step in.

I agree caching should be handled in a uniform and consistent way. The only reason to put memcached server(s) settings inside the Apache config is to be able to cache de koha-conf.xml file itself. This has been discussed on bug 6193. Otherwise, it can be put anywhere (koha-conf.xml or db).

There's also a second patch in bug 6193, that reuses those settings read from ENV, and the C4::Context->memcached object itself (and thus removes lots of calls to sysprefs in several koha libraries too).

Regarding the 'special-non-cached' syspref usecache, it could be tested before setting the memcached stuff in C4::Context and thus provide a way to disable caching for the librarians.

I sent an email to the list to discuss caching, as we have two different ways of dealing with it right now, and not a proper solution to cache invalidation, which actually annoys several librarians here (think of languages, changes on frameworks, etc). I hope we can discuss it on the list soon.
Comment 27 Paul Poulain 2012-02-21 12:44:08 UTC
Following discussion & decision on koha-devel, the current patch won't be applied : we have decided to set caching at apache configuration level, bug 6193 has been applied and provide this feature/level of caching.

So the "usecache" syspref is now irrelevent (we use cache BEFORE reading syspref)
Using Koha/Cache is probably a good idea though, but the patch must be rewritten completly imo.

For now, all caching is made with the following code:

eval {
    if (C4::Context->ismemcached) {
        require Memoize::Memcached;
        import Memoize::Memcached qw(memoize_memcached);
        memoize_memcached('getTranslatedLanguages', memcached => C4::Context->memcached);
}

->ismemcached answer 1 if memcache is ON
C4::Context->memcached returns the handle of the memcache server

Some ideas behing this patch are great and should be reintroduced:
* use a central/unique Caching place = will let us change the caching system if we want, it's something we can't do with current system
* the cache flushing is something we must add, definetly.

So you're more than welcomed to rewrite your patch !
Comment 28 Katrin Fischer 2012-02-26 17:15:59 UTC
I would really like to see this feature in 3.8. I hope it can be fixed and will be happy to test again.
Comment 29 Jared Camins-Esakov 2012-04-29 13:15:03 UTC Comment hidden (obsolete)
Comment 30 Jared Camins-Esakov 2012-04-29 13:15:46 UTC Comment hidden (obsolete)
Comment 31 Katrin Fischer 2012-04-29 19:41:52 UTC Comment hidden (obsolete)
Comment 32 Katrin Fischer 2012-04-29 19:42:07 UTC Comment hidden (obsolete)
Comment 33 Mason James 2012-04-30 01:58:46 UTC
Created attachment 9360 [details] [review]
Bug 7248 Added caching support and and moved Caching into Koha namespace

Unit test for Koha/Cache.pm (which includes Koha/Cache/Memcached.pm)

Note that in order to test Koha::Cache you must export the environment variable
MEMCACHED_SERVERS. For example:
$ export MEMCACHED_SERVERS=127.0.0.1:11211

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 34 Mason James 2012-04-30 02:15:27 UTC
passing QA - passes perlcritic OK, and both tests pass OK

i've squashed the two patches into one, 
applied to 142c62081fc15bbd994e737eef55e29b61a6d730
Comment 35 Paul Poulain 2012-05-14 08:20:50 UTC
Looking at how this caching works in bug 7249, I think this part of the code is wrong: 

+if ($usecache) {
+    require Koha::Cache;
+    Koha::Cache->import();
+    $cache = Koha::Cache->new(
+        {
+            'cache_type'    => 'memcached',
+            'cache_servers' => $ENV{'MEMCACHED_SERVERS'}
+        }
+    );
+    my $namespace = $ENV{'MEMCACHED_NAMESPACE'} || 'koha';
+    my $page = $cache->get_from_cache("$namespace:intranet:report:$report");
+    if ($page) {
+        print $query->header;
+        print $page;
+        exit;
+    }
+}

=> 'memcached' is hardcoded in the file that use caching, the caching mechanism must be generic.

The caching API must be something like:
use Koha::Cache;
my $cache_server = Koha::Cache->new();
if $cache_server( my $page = $cache->get_from_cache("intranet:report:$report");)

so it won't depend on the caching system & caching configuration.

I'll provide a patch for that ASAP (maybe today)
Comment 36 Katrin Fischer 2012-05-14 08:33:09 UTC
Hi Paul,
I understand what you mean, but couldn't you still push this? I see some reasons:
- adding new reports is currently totally broken in master
- I am quite sure we have more references for memcache that need to be cleaned up
- Koha does only support memcache right now, so it can be pushed without breaking anything
- the change you suggest could be done as a follow-up and perhaps do more clean-up in other places at the same time
Comment 37 Paul Poulain 2012-05-14 09:29:42 UTC Comment hidden (obsolete)
Comment 38 Paul Poulain 2012-05-14 09:46:34 UTC
(wait an hour or 2 before testing this patch, i'm working on t/Cache.t and cleaning other instances of caching)
Comment 39 Paul Poulain 2012-05-14 11:27:56 UTC Comment hidden (obsolete)
Comment 40 Paul Poulain 2012-05-14 11:28:53 UTC Comment hidden (obsolete)
Comment 41 Paul Poulain 2012-05-14 11:29:03 UTC Comment hidden (obsolete)
Comment 42 Paul Poulain 2012-05-14 11:41:35 UTC Comment hidden (obsolete)
Comment 43 Paul Poulain 2012-05-14 11:43:28 UTC
If anyone can sign-off my 3 follow-ups...

there's only one missing, to remove ismemcache from Context.pm and Auth.pm, will do that once those have been pushed.

Then, i'll write a wiki page about caching
Comment 44 Paul Poulain 2012-05-14 15:30:30 UTC Comment hidden (obsolete)
Comment 45 Paul Poulain 2012-05-14 15:34:36 UTC Comment hidden (obsolete)
Comment 46 Paul Poulain 2012-05-14 15:45:48 UTC Comment hidden (obsolete)
Comment 47 Jared Camins-Esakov 2012-05-14 15:57:13 UTC
Created attachment 9571 [details] [review]
Bug 7248 follow-up (alternative)

This patch introduces some new features for caching system:
* the type of caching is retrieved from ENV variable in httpd configuration if not forced when the ->new() is called.
* if $ENV{DEBUG} is ON, you'll have feedback when something is set or read from cache
* the Koha::Cache->is_cache_active is now available and will return 1 if there is a caching system available. It's a replacement for C4::Context->ismemcached
* fixes the mistake for debug/compress_threshold and expire_time parameters

The 2 report web service modules have been updated to use this new API

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Comment 48 Ian Walls 2012-05-15 16:04:03 UTC
Followup breaks out requirement of Koha::Cache to a universal use, then makes use of the new subroutine.  Also centralizes the cache params into Koha::Cache->new().  Also adds a pre-emptive check for ENV variables indicating caching; if not present, will continue to fall back to other methods.  Some additional warns if DEBUG is active.

All in all, looks okay, and applies cleanly (provided you apply the baseline patch first).  Marking Passed QA.
Comment 49 Jared Camins-Esakov 2012-12-31 00:59:22 UTC
There have been no further reports of problems so I am marking this bug resolved.