Bug 14522 - Use Koha::Cache for accessing single_holidays()
Summary: Use Koha::Cache for accessing single_holidays()
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Mason James
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 8089 14315 15333 16539
  Show dependency treegraph
 
Reported: 2015-07-13 06:49 UTC by Mason James
Modified: 2016-12-05 21:27 UTC (History)
8 users (show)

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


Attachments
Bug 14522 - Use Koha::Cache for accessing single_holidays() (21.08 KB, patch)
2015-07-17 03:54 UTC, Mason James
Details | Diff | Splinter Review
pic: create range of holidays (147.99 KB, image/png)
2015-09-03 01:52 UTC, Mason James
Details
Bug 14522 - Use Koha::Cache for accessing single_holidays() (21.10 KB, patch)
2015-09-24 18:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14522 - Use Koha::Cache for accessing single_holidays() (21.12 KB, patch)
2015-09-25 01:42 UTC, Mason James
Details | Diff | Splinter Review
fix =cut typo (706 bytes, patch)
2015-09-25 01:45 UTC, Mason James
Details | Diff | Splinter Review
Bug 14522 [Followup] - fix =cut typo (695 bytes, patch)
2015-09-25 17:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14522 [QA Followup] - Koha complains about missing C4::Dates on master (597 bytes, patch)
2015-09-25 17:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 14522 - Use Koha::Cache for accessing single_holidays() (21.16 KB, patch)
2015-09-28 14:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 14522 [Followup] - fix =cut typo (763 bytes, patch)
2015-09-28 14:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 14522 [QA Followup] - Koha complains about missing C4::Dates on master (664 bytes, patch)
2015-09-28 14:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 14522 [QA Followup] - Remove $single_holidays global (805 bytes, patch)
2015-09-28 14:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Script to try and reproduce the problem (891 bytes, application/x-perl)
2015-09-28 14:50 UTC, Jonathan Druart
Details
Bug 14522: (QA followup) tidy tests (6.72 KB, patch)
2015-10-02 14:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14522 [QA Followup] (1.71 KB, patch)
2015-10-02 15:02 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 Mason James 2015-07-13 06:49:33 UTC
more info to come
Comment 1 Mason James 2015-07-17 03:54:14 UTC Comment hidden (obsolete)
Comment 2 j.kylmala 2015-09-01 06:16:10 UTC
(In reply to Mason James from comment #1)
>1/ add a bunch of single_holidays to your test koha, (my table has 400 holiday >rows)

Could you give some example table? It would be easier to test.

>2/ add a loong circ rule for an itemtype (my rule has 140 days)

Do you mean loan period?
Comment 3 Mason James 2015-09-03 01:50:27 UTC
(In reply to Joonas Kylmälä from comment #2)
> (In reply to Mason James from comment #1)
> >1/ add a bunch of single_holidays to your test koha, (my table has 400 holiday >rows)
> 
> Could you give some example table? It would be easier to test.

hmm, yes i could, but...

 you can quickly create a range of special/unique holidays in a few clicks
... from '2014-01-01' to '2016-01-01', would give you 730 holidays for testing

i'll attach a pic

> 
> >2/ add a loong circ rule for an itemtype (my rule has 140 days)
> 
> Do you mean loan period?

oops yes, thats what i mean :)
Comment 4 Mason James 2015-09-03 01:52:07 UTC
Created attachment 42255 [details]
pic: create range of holidays
Comment 5 Mason James 2015-09-03 01:54:47 UTC
(In reply to Mason James from comment #3)
> (In reply to Joonas Kylmälä from comment #2)
> > (In reply to Mason James from comment #1)
> > >1/ add a bunch of single_holidays to your test koha, (my table has 400 holiday >rows)
> > 
> > Could you give some example table? It would be easier to test.
> 
> hmm, yes i could, but...
> 
>  you can quickly create a range of special/unique holidays in a few clicks
> ... from '2014-01-01' to '2016-01-01', would give you 730 holidays for
> testing
> 
> i'll attach a pic

mysql> select count(*)  from special_holidays where title = 'aaa' ;
+----------+
| count(*) |
+----------+
|      614 |
+----------+
1 row in set (0.00 sec)
Comment 6 James 2015-09-03 05:57:57 UTC
Problem in testing, when trying to create holidays using patch the date format is being read incorrectly resulting in the following message for a holiday range ending on 26th November:

Software error:

The 'month' parameter ("26") to DateTime::new did not pass the 'an integer between 1 and 12' callback
 at /usr/lib/perl5/DateTime.pm line 199
	DateTime::new(undef, 'year', 2015, 'month', 26, 'day', 11) called at /home/koha/src/tools/newHolidays.pl line 67
Comment 7 Mason James 2015-09-22 01:40:56 UTC
(In reply to James from comment #6)
> Problem in testing, when trying to create holidays using patch the date
> format is being read incorrectly resulting in the following message for a
> holiday range ending on 26th November:
> 
> Software error:
> 
> The 'month' parameter ("26") to DateTime::new did not pass the 'an integer
> between 1 and 12' callback
>  at /usr/lib/perl5/DateTime.pm line 199
> 	DateTime::new(undef, 'year', 2015, 'month', 26, 'day', 11) called at
> /home/koha/src/tools/newHolidays.pl line 67

Hmm, it looks like I might have only tested this patch with a 'Y-m-d' DateFormat syspref value.

I'll take another look at this
Comment 8 Mason James 2015-09-22 03:06:43 UTC
(In reply to James from comment #6)
> Problem in testing, when trying to create holidays using patch the date
> format is being read incorrectly resulting in the following message for a
> holiday range ending on 26th November:
> 
> Software error:
> 
> The 'month' parameter ("26") to DateTime::new did not pass the 'an integer
> between 1 and 12' callback
>  at /usr/lib/perl5/DateTime.pm line 199
> 	DateTime::new(undef, 'year', 2015, 'month', 26, 'day', 11) called at
> /home/koha/src/tools/newHolidays.pl line 67


hmm, this bug seems to be caused by...

1/ loading the /cgi-bin/koha/tools/holidays.pl page
2/ changing the 'dateformat' syspref
3/ creating a new holiday, without first reloading the existing holidays.pl page

..as this sequence of events is quite rare, and would only occur during deliberate testing - i think this bug is a effectively a WONTFIX

FYI: this bug is reproducible without this patch, and has likely existed since the introduction of the calendar/holidays feature in 2006.
Comment 9 Mason James 2015-09-22 03:39:55 UTC
> ..as this sequence of events is quite rare, and would only occur during
> deliberate testing - i think this bug is a effectively a WONTFIX

Sorry, when I say WONTFIX - I mean to say, the error that James experienced can be ignored for this bug

i don't mean that this bug itself is a WONTFIX :)
Comment 10 Nick Clemens 2015-09-24 18:00:39 UTC Comment hidden (obsolete)
Comment 11 Nick Clemens 2015-09-24 18:02:08 UTC
Test plan and all tests pass.  QA test tools fail for pod in Koha::Calendar.pm

Line 97 looks like you said =c when you meant =cut
Comment 12 Mason James 2015-09-25 01:42:17 UTC Comment hidden (obsolete)
Comment 13 Mason James 2015-09-25 01:44:00 UTC
Comment on attachment 42855 [details] [review]
Bug 14522 - Use Koha::Cache for accessing single_holidays()

oops
Comment 14 Mason James 2015-09-25 01:45:12 UTC Comment hidden (obsolete)
Comment 15 Mason James 2015-09-25 01:49:41 UTC
(In reply to Nick Clemens from comment #11)
> Test plan and all tests pass.  QA test tools fail for pod in
> Koha::Calendar.pm
> 
> Line 97 looks like you said =c when you meant =cut

thanks, i attached a new patch to fix that..

mason@xen1:~/g/k/master$ /home/mason/git/koha-qa-tools/koha-qa.pl  -c 2
testing 2 commit(s) (applied to f38f8b8 'Fixing release notes')

Processing files before patches
|========================>| 5 / 5 (100.00%)
Processing files after patches
|========================>| 5 / 5 (100.00%)

 OK     Koha/Calendar.pm
 OK     C4/Calendar.pm
 OK     t/Calendar.t
 OK     t/db_dependent/Holidays.t
 OK     tools/newHolidays.pl
Comment 16 Kyle M Hall 2015-09-25 17:44:28 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2015-09-25 17:44:37 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2015-09-25 17:47:32 UTC
My testing has not shown any regressions, but I've been unable to prove any speed improvement. Maston, can you provide a unit test or even just a perl script to demonstrate the speedup? I set up about 350 holidays and tried checkout periods of 180 and 9999 days. Both were about the same speed with and without the patches respectively. I do have memcached installed and configured for Koha.

memcdump --servers localhost
koha_kohaqaismemcached
koha_kohaqasingle_holidays
koha_kohaqaAuthorisedValues-BOR_NOTES--0-CPL
koha_kohaqaColumnsSettingsYaml
koha_kohaqakohaconf


Looks like they are getting cached!
Comment 19 Mason James 2015-09-26 11:32:58 UTC
(In reply to Kyle M Hall from comment #18)
> My testing has not shown any regressions, but I've been unable to prove any
> speed improvement. Maston, can you provide a unit test or even just a perl
> script to demonstrate the speedup? 

i have neither of those

> I set up about 350 holidays and tried
> checkout periods of 180 and 9999 days. Both were about the same speed with
> and without the patches respectively. 

your circ speed will only be slow if you test against a patron who has a branch with many holidays *and* an item with a matching long loan period rule
Comment 20 Kyle M Hall 2015-09-28 14:24:10 UTC
> > I set up about 350 holidays and tried
> > checkout periods of 180 and 9999 days. Both were about the same speed with
> > and without the patches respectively. 
> 
> your circ speed will only be slow if you test against a patron who has a
> branch with many holidays *and* an item with a matching long loan period rule

Confirmed! I think the issue was my rule was selecting a library other than the one my holidays were set for!
Comment 21 Kyle M Hall 2015-09-28 14:24:45 UTC
Created attachment 42919 [details] [review]
[PASSED QA] Bug 14522 - Use Koha::Cache for accessing single_holidays()

this patch adds Koha::Cache functionality to the 'single_holidays' table
it is a performance patch for the problem described in BZ14315, only

 http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14315

it replaces slooow DateTime holiday objects with simple Ymd strings (19991230), then stores the strings in an @array using Koha::Cache
it does not attempt to add caching to all holiday tables - just the single_holidays table (at this stage

 on my test (master-cd9a827); nytprof showed a time reduction of the single_holidays() sub - from 61.7s to 587ms

here are some before/after nytprof runs, (really on master-cd9a827, not 3.20)

 http://x1.kohaaloha.com/i/nyt-bz14522-before/home-mason-g-k-3-20-x-Koha-Calendar-pm-1485-line.html#237
 http://x1.kohaaloha.com/i/nyt-bz14522-after/home-mason-g-k-3-20-x-Koha-Calendar-pm-1485-line.html#280

to test...

1/ add a bunch of single_holidays to your test koha, (my table has 400 holiday rows)
2/ add a loong circ rule for an itemtype (my rule has 140 days)
3/ checkout an item to a user (took me 67 secs)

apply patch...
4/ return item
5/ repeats steps 1..3, (took me 6 secs)

6/ add/change/delete some various single_holidays, via Home->Tools->Calendar
    ensure that your various changes have indeed saved correctly

for extra points...

7/ run tests t/Calendar.t and t/db_dependent/Holidays.t, with all tests pass OK

 sudo  koha-shell -c '  export PERL5LIB=/home/mason/g/k/master ; \
 cd /home/mason/g/k/master ;  perl  t/Calendar.t ; perl  t/db_dependent/Holidays.t  '   testkoha

8/ run QA tool, with all tests pass OK

 sudo  koha-shell -c ' \
export KOHA_CONF=/etc/koha/sites/mayo2/koha-conf.xml \
 export PERL5LIB=/home/mason/g/k/master:/home/mason/qa-test-tools/ ; \
 cd /home/mason/g/k/master ;   perl /home/mason/qa-test-tools/koha-qa.pl  -c 1 ' testkoha

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2015-09-28 14:24:53 UTC
Created attachment 42920 [details] [review]
[PASSED QA] Bug 14522 [Followup] - fix =cut typo

http://bugs.koha-community.org/show_bug.cgi?id=14522

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2015-09-28 14:24:55 UTC
Created attachment 42921 [details] [review]
[PASSED QA] Bug 14522 [QA Followup] - Koha complains about missing C4::Dates on master

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Kyle M Hall 2015-09-28 14:24:58 UTC
Created attachment 42922 [details] [review]
[PASSED QA] Bug 14522 [QA Followup] - Remove $single_holidays global

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Jonathan Druart 2015-09-28 14:49:49 UTC
Comment on attachment 42919 [details] [review]
[PASSED QA] Bug 14522 - Use Koha::Cache for accessing single_holidays()

Review of attachment 42919 [details] [review]:
-----------------------------------------------------------------

It seems we were here at the same time :)

I have also tried to reproduce the time reduction, without success.
A script I have used is coming...

::: Koha/Calendar.pm
@@ +94,5 @@
>      my $branchcode = $self->{branchcode};
> +    my $cache           = Koha::Cache->get_instance();
> +    my $single_holidays = $cache->get_from_cache('single_holidays');
> +
> +=c

Should be a simple comment, no need to put it in the pod.

@@ +395,5 @@
> +    $single_holidays->{'MPL'} = [$ymd];
> +    $cache->set_in_cache( 'single_holidays', $single_holidays, 76800 );
> +
> +    # ...but *dont* reset the cache, as this holiday was not really written to the db
> +    # its only used to mock a holiday insert for 1 test in t/db_dependent/Holidays.t

Should not be a method here, but moved to the test file or a test module.
Comment 26 Jonathan Druart 2015-09-28 14:50:44 UTC
Created attachment 42926 [details]
Script to try and reproduce the problem
Comment 27 Tomás Cohen Arazi 2015-10-02 14:25:41 UTC
I like the implementation (overall) but I don't think we should introduce things like Koha::Calendar::add_dummy_holiday. Worse, it relies on the 'MPL' branch existing on the database.
Comment 28 Tomás Cohen Arazi 2015-10-02 14:44:57 UTC
Created attachment 43070 [details] [review]
Bug 14522: (QA followup) tidy tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Comment 29 Kyle M Hall 2015-10-02 15:02:25 UTC
Created attachment 43071 [details] [review]
Bug 14522 [QA Followup]

* Change POD to a brief comment
* Fix UT so it doesn't fail on Sundays
Comment 30 Tomás Cohen Arazi 2015-10-02 15:12:14 UTC
Patches pushed to master.

Thanks Mason and Kyle!
Comment 31 Jonathan Druart 2015-10-05 07:32:54 UTC
(In reply to Jonathan Druart from comment #25)
> Comment on attachment 42919 [details] [review] [review]
> [PASSED QA] Bug 14522 - Use Koha::Cache for accessing single_holidays()
> 
> Review of attachment 42919 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> It seems we were here at the same time :)
> 
> I have also tried to reproduce the time reduction, without success.
> A script I have used is coming...

It would have been good to have a script to highlight the time reduction of this caching.