Bug 12041 - Improve Koha::Cache
Summary: Improve Koha::Cache
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: Robin Sheat
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 9967 11842 12173
  Show dependency treegraph
 
Reported: 2014-04-07 23:33 UTC by Robin Sheat
Modified: 2015-12-03 22:11 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 12041 - improve Koha::Cache (15.78 KB, patch)
2014-04-08 05:57 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 12041 - utf8 compatibility changes (3.37 KB, patch)
2014-04-09 02:47 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 12041 - improve Koha::Cache (16.44 KB, patch)
2014-04-09 02:49 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 12041 - improve Koha::Cache (16.70 KB, patch)
2014-04-10 05:20 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 12041 - improve Koha::Cache (16.69 KB, patch)
2014-04-22 05:13 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 12041 - POD fixing followup (1.23 KB, patch)
2014-05-22 00:01 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 12041 - improve Koha::Cache (16.82 KB, patch)
2014-06-02 13:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12041 - POD fixing followup (1.29 KB, patch)
2014-06-02 13:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12041: UT - Get rid of warnings (906 bytes, patch)
2014-06-02 13:56 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Sheat 2014-04-07 23:33:37 UTC
Koha::Cache can be improved to fall back more reliably, to handle complex data types, and so on. This is necessary to allow caching of things properly under plack.
Comment 1 Robin Sheat 2014-04-08 05:57:16 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2014-04-08 07:46:41 UTC
(In reply to Robin Sheat from comment #1)
> Created attachment 26891 [details] [review] [review]
> Bug 12041 - improve Koha::Cache
> 
> This makes Koha::Cache behave better by default. It will use memcached
> if available to do shared caching, if that's not available it will fall
> back to in-memory caching. It also allows for a singleton accessor to
> allow a single cache to be shared within a process.
> 
> Test plan:
> * The t/Cache.t file runs successfully with and without the
>   MEMCACHED_SERVERS envvar set (and memcached running in the
>   environment.)

You will want to test that storing utf8 and retrieving it still works. If this change munches utf8 then it makes the caching unusable in a lot of situations.
Comment 3 Robin Sheat 2014-04-09 02:47:08 UTC Comment hidden (obsolete)
Comment 4 Robin Sheat 2014-04-09 02:49:18 UTC Comment hidden (obsolete)
Comment 5 Robin Sheat 2014-04-10 05:20:58 UTC Comment hidden (obsolete)
Comment 6 Robin Sheat 2014-04-22 05:13:41 UTC Comment hidden (obsolete)
Comment 7 Brendan Gallagher 2014-05-21 22:39:15 UTC
Robin - 

		*** ERROR: 
		 
		Spurious text after =cut
		=over on line 219 without closing =back (at head2)
		 in file Koha/Cache.pm

Pretty simple clean up.  I tested and it's working, happy to signoff with the follow up for that - or I can ignore that and just tell the QA to follow up or the RM to followup when pushing.  let me know what to do.

-Brendan
Comment 8 Robin Sheat 2014-05-22 00:01:29 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2014-05-22 13:55:13 UTC
Robin,

All looks good but I don't understand something in the current code.
In Koha/Cache.pm

458     unless ( lc( $args->{cache_type} ) eq 'null' ) {
459         $args->{'cache'} = $self;
460         $args->{'cache_type'} ||= $ENV{'CACHING_SYSTEM'};
461     }

Isn't it $self->{cache_type} instead of $args->{cache_type}?
Comment 10 Robin Sheat 2014-05-23 01:39:01 UTC
(In reply to Jonathan Druart from comment #9)
> Isn't it $self->{cache_type} instead of $args->{cache_type}?

I don't think so, it's just figuring out the defaults and adding them to the $args hashref. That's then passed on to Koha::Cache::Object when it's tied.
Comment 11 Jonathan Druart 2014-06-02 13:56:31 UTC
Created attachment 28611 [details] [review]
Bug 12041 - improve Koha::Cache

This makes Koha::Cache behave better by default. It will use memcached
if available to do shared caching, if that's not available it will fall
back to in-memory caching. It also allows for a singleton accessor to
allow a single cache to be shared within a process.

* Added tests to confirm UTF8-cleanness.
* Added minor fixups to stop warnings.

Test plan:
* The t/Cache.t file runs successfully with and without the
  MEMCACHED_SERVERS envvar set (and memcached running in the
  environment.)

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 12 Jonathan Druart 2014-06-02 13:56:35 UTC
Created attachment 28612 [details] [review]
Bug 12041 - POD fixing followup

This fixes a minor POD error detected, and also adds the Koha/ directory
to the regular test case so that the in-repo tests can find these
things.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 13 Jonathan Druart 2014-06-02 13:56:39 UTC
Created attachment 28613 [details] [review]
Bug 12041: UT - Get rid of warnings

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 14 Tomás Cohen Arazi 2014-06-19 16:07:33 UTC
Pushed to master.

Thanks Robin!
Comment 15 Jonathan Druart 2015-02-10 11:59:21 UTC
This patch set changed the prototype of set_in_cache, but did not update the calls in t/Cache.t.