Bug 7432

Summary: Changing frameworks should refresh cache
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: System AdministrationAssignee: Paul Poulain <paul.poulain>
Status: CLOSED FIXED QA Contact: Ian Walls <koha.sekjal>
Severity: critical    
Priority: P1 - high CC: chris, gmcharlt, jcamins
Version: 3.6   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 7432 : Fix how we are setting expiry time when caching
Bug 7432 : Fixing caching for SQLHelper
Bug 7432 : Fixing caching for SQLHelper
Bug 7432 : Fixing caching for C4::Languages
Bug 7432 : Fix how we are setting expiry time when caching
Bug 7432 : Fixing caching for SQLHelper
Bug 7432 : Fixing caching for C4::Languages

Description Katrin Fischer 2012-01-10 07:21:53 UTC
I see several small problems when using memcache and making changes to the bibliographic frameworks:

Changes are not reflected immediately, but only after restarting the memcache server. This is a problem, because often you want to configure and immediately check if your changes are correct by refreshing the cataloging screen. At the moment this is really confusing. I am not sure when or if the cache would be updated without restarting memcache.

To reproduce:
1) Open cataloguing default framework
2) Change bibliographic framework, for example: set 773$a hidden value from -6 to 0
3) Refresh cataloguing page > change and 773 will not be visible
4) Restart memcache > change will be visible
Comment 1 Chris Cormack 2012-02-02 23:32:53 UTC
This is a bug in the way we are using memoize_memcached

I will do patches to fix it in all the places we use it.

The first one will be for C4/Biblio and will show what we are doing wrong.
Comment 2 Chris Cormack 2012-02-02 23:36:24 UTC
Now they will only be cached for 10 mins max, so will reflect the change without needing a restart. I will do this for all instances of memoize_memcached and attach them here.
Comment 3 Chris Cormack 2012-02-02 23:36:54 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2012-02-02 23:41:17 UTC Comment hidden (obsolete)
Comment 5 Chris Cormack 2012-02-02 23:43:13 UTC Comment hidden (obsolete)
Comment 6 Chris Cormack 2012-02-02 23:44:51 UTC Comment hidden (obsolete)
Comment 7 Ian Walls 2012-02-03 23:48:55 UTC
Looks like all these patches fold the expire_time into the memcached hash, instead of as as separate param to memoize_memcached().  That's what's expected by the function according to CPAN, so I think this is a valid change.

Chris, your commit messages for these changes pretty empty. Could you provide a  test plan so we can confirm this is working as expected?
Comment 8 Chris Cormack 2012-02-04 00:59:30 UTC
(In reply to comment #7)
> Looks like all these patches fold the expire_time into the memcached hash,
> instead of as as separate param to memoize_memcached().  That's what's expected
> by the function according to CPAN, so I think this is a valid change.
> 
> Chris, your commit messages for these changes pretty empty. Could you provide a
>  test plan so we can confirm this is working as expected?

Seriously?

Ok, switch on caching, go to cataloguing, pick a framework, look at it. Change the framework, check it again, it wont have have changed in cataloguing. Wait 10 minutes look again.

Currently it never ever changes unless you restart memcached.

Katrin already has the test plan, use that, except that after 10 minutes the change will show instead of having to restart memcached.
Comment 9 Jared Camins-Esakov 2012-02-12 19:09:47 UTC
Created attachment 7585 [details] [review]
Bug 7432 : Fix how we are setting expiry time when caching

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Comment 10 Jared Camins-Esakov 2012-02-12 19:10:06 UTC
Created attachment 7586 [details] [review]
Bug 7432 : Fixing caching for SQLHelper

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Comment 11 Jared Camins-Esakov 2012-02-12 19:10:19 UTC
Created attachment 7587 [details] [review]
Bug 7432 : Fixing caching for C4::Languages

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Comment 12 Paul Poulain 2012-02-15 09:38:04 UTC
2 patches set
> expire_time => 600
saying it's 10mn
One patch (http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=7586) set
> expire_time => 600000
also saying it's 10mn

I suspect the 60000 is a bug, marking failed QA: pls explain or follow-up, thx
(I tried to dig into Cache::Memcached documentation, and found:

The $exptime (expiration time) defaults to "never" if unspecified.  If you want the key to expire in memcached, pass an integer $exptime.  If value is less than 60*60*24*30 (30 days), time is assumed to be relative from the present.  If larger, it's considered an absolute Unix time.
Comment 13 Chris Cormack 2012-02-15 09:41:18 UTC
Yes the 60000 is a bug, thats why i changed it to 600 (someone elses patch had set it to 60000 so i fixed it while fixing the fact it was set in the wrong place)

If you look carefully this patch is changing it from 60000 to 600.
Comment 14 Paul Poulain 2012-02-15 09:45:44 UTC
OK, /me must change his glasses...

passed QA
Comment 15 Jared Camins-Esakov 2012-05-23 13:01:18 UTC
This fix was included in the 3.6.x branch prior to 3.6.4.