[Bug 33447] New: Add caching to Biblio->pickup_locations
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Bug ID: 33447 Summary: Add caching to Biblio->pickup_locations Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: nick@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org Biblio->pickup_locations iterates over each item and calls Item->pickup_locations for each The routine uses a maximum of 5 data points: homebranch, holdingbranch, itype (specifically, not effective_itemtype), ccode (if using branch transfer limits by ccode), and patron branchcode We can add L1 caching on this call to reduce simple repeated calls -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 --- Comment #1 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 149295 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=149295&action=edit Bug 33447: Add Cache to Biblio->pickup_locations This is going to have the most effect on records with large numbers of items held by the same library, serial records and the like To test: 1 - Add 500 items to a biblio by select myltiple copies on the add item page 2 - Place a hold via the API and note response time, I found ~3-5 seconds 3 - Apply patch 4 - Restart all 5 - Place hold using api again 6 - Note improved response time, less than 1/2 a second in my tests -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 --- Comment #2 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 149296 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=149296&action=edit Bug 33447: Adjust tests -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com, tomascohen@gmail.com Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |michaela.sieber@kit.edu -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |33471 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33471 [Bug 33471] Improve performance of hold pickup location verification for next available holds -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #149295|0 |1 is obsolete| | --- Comment #3 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 149389 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=149389&action=edit Bug 33447: Add Cache to Biblio->pickup_locations This is going to have the most effect on records with large numbers of items held by the same library, serial records and the like To test: 1 - Add 500 items to a biblio by select myltiple copies on the add item page 2 - Place a hold via the API and note response time, I found ~3-5 seconds 3 - Apply patch 4 - Restart all 5 - Place hold using api again 6 - Note improved response time, less than 1/2 a second in my tests Signed-off-by: emlam <emily.lamancusa@montgomerycountymd.gov> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #149296|0 |1 is obsolete| | --- Comment #4 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 149390 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=149390&action=edit Bug 33447: Adjust tests Signed-off-by: emlam <emily.lamancusa@montgomerycountymd.gov> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |emily.lamancusa@montgomeryc | |ountymd.gov Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- I think that it would be a very good thing to try caching here. What I am not sure about, is the theoretical possibility of getting wrong results if we would repeatedly ask for pickup_locations (while handling one request). The change in the test confirms this actually. You do not add a test for caching, but just add a flush. Removing that one makes the test fail. # Subtest: pickup_locations not ok 6 - ReservesControlBranch: PatronLibrary, biblio1, patron8 should return 3 but returns 1 not ok 7 - ReservesControlBranch: PatronLibrary, biblio2, patron1 should return 0 but returns 2 not ok 8 - ReservesControlBranch: PatronLibrary, biblio2, patron8 should return 3 but returns 0 # Looks like you failed 3 tests of 9. not ok 8 - pickup_locations I would recommend to add the flush in Biblio->pickup_locations. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #6 from Nick Clemens <nick@bywatersolutions.com> --- As I understood, the L1 and Memory Lite caches are flushed on each request: /etc/koha/sites/kohadev/plack.psgi: use CGI qw(-utf8 ); # we will loose -utf8 under plack, otherwise { no warnings 'redefine'; my $old_new = \&CGI::new; *CGI::new = sub { my $q = $old_new->( @_ ); $CGI::PARAM_UTF8 = 1; Koha::Caches->flush_L1_caches(); Koha::Cache::Memory::Lite->flush(); return $q; }; } Am I not understanding correctly? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 --- Comment #7 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Marcel de Rooy from comment #5)
I think that it would be a very good thing to try caching here. What I am not sure about, is the theoretical possibility of getting wrong results if we would repeatedly ask for pickup_locations (while handling one request).
(In reply to Nick Clemens from comment #6)
As I understood, the L1 and Memory Lite caches are flushed on each request:
Am I not understanding correctly?
Sure you do. But see above. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 --- Comment #8 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Marcel de Rooy from comment #5)
I think that it would be a very good thing to try caching here. What I am not sure about, is the theoretical possibility of getting wrong results if we would repeatedly ask for pickup_locations (while handling one request).
The change in the test confirms this actually. You do not add a test for caching, but just add a flush. Removing that one makes the test fail.
# Subtest: pickup_locations not ok 6 - ReservesControlBranch: PatronLibrary, biblio1, patron8 should return 3 but returns 1 not ok 7 - ReservesControlBranch: PatronLibrary, biblio2, patron1 should return 0 but returns 2 not ok 8 - ReservesControlBranch: PatronLibrary, biblio2, patron8 should return 3 but returns 0 # Looks like you failed 3 tests of 9. not ok 8 - pickup_locations
I would recommend to add the flush in Biblio->pickup_locations.
Those tests fail because we have changed a syspref - we are essentially testing multiple requests there. It is not expected for a syspref to change during a request. I flush to indicate that -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Nick Clemens from comment #8)
(In reply to Marcel de Rooy from comment #5)
I think that it would be a very good thing to try caching here. What I am not sure about, is the theoretical possibility of getting wrong results if we would repeatedly ask for pickup_locations (while handling one request).
The change in the test confirms this actually. You do not add a test for caching, but just add a flush. Removing that one makes the test fail.
# Subtest: pickup_locations not ok 6 - ReservesControlBranch: PatronLibrary, biblio1, patron8 should return 3 but returns 1 not ok 7 - ReservesControlBranch: PatronLibrary, biblio2, patron1 should return 0 but returns 2 not ok 8 - ReservesControlBranch: PatronLibrary, biblio2, patron8 should return 3 but returns 0 # Looks like you failed 3 tests of 9. not ok 8 - pickup_locations
I would recommend to add the flush in Biblio->pickup_locations.
Those tests fail because we have changed a syspref - we are essentially testing multiple requests there. It is not expected for a syspref to change during a request. I flush to indicate that
Thx for explaining. Would be good to add a short comment for such things. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Marcel de Rooy from comment #5)
I think that it would be a very good thing to try caching here. What I am not sure about, is the theoretical possibility of getting wrong results if we would repeatedly ask for pickup_locations (while handling one request).
I would recommend to add the flush in Biblio->pickup_locations.
Apart from the syspref issue, this comment still got a bit out of sight. The theoretical(!) chance of calling it multiple times within one(!) request.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 --- Comment #11 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Marcel de Rooy from comment #10)
(In reply to Marcel de Rooy from comment #5)
I think that it would be a very good thing to try caching here. What I am not sure about, is the theoretical possibility of getting wrong results if we would repeatedly ask for pickup_locations (while handling one request).
I would recommend to add the flush in Biblio->pickup_locations.
Apart from the syspref issue, this comment still got a bit out of sight. The theoretical(!) chance of calling it multiple times within one(!) request..
Multiple times is fine, we don't expect sysprefs to change within a single request - even for different bibs the results are not different for the same type of item -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Nick Clemens from comment #11)
(In reply to Marcel de Rooy from comment #10)
(In reply to Marcel de Rooy from comment #5)
I think that it would be a very good thing to try caching here. What I am not sure about, is the theoretical possibility of getting wrong results if we would repeatedly ask for pickup_locations (while handling one request).
I would recommend to add the flush in Biblio->pickup_locations.
Apart from the syspref issue, this comment still got a bit out of sight. The theoretical(!) chance of calling it multiple times within one(!) request..
Multiple times is fine, we don't expect sysprefs to change within a single request - even for different bibs the results are not different for the same type of item
I had in mind to be depending on some patron attribute too, but it looks good. In pickup_locations and related calls, we only seem to actually use branchcode. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #149389|0 |1 is obsolete| | --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 149790 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=149790&action=edit Bug 33447: Add Cache to Biblio->pickup_locations This is going to have the most effect on records with large numbers of items held by the same library, serial records and the like To test: 1 - Add 500 items to a biblio by select myltiple copies on the add item page 2 - Place a hold via the API and note response time, I found ~3-5 seconds 3 - Apply patch 4 - Restart all 5 - Place hold using api again 6 - Note improved response time, less than 1/2 a second in my tests Signed-off-by: emlam <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #149390|0 |1 is obsolete| | --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 149791 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=149791&action=edit Bug 33447: Adjust tests Signed-off-by: emlam <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 149792 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=149792&action=edit Bug 33447: (QA follow-up) Add comment for flushing cache Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |23.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 --- Comment #16 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.05. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 --- Comment #17 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 149968 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=149968&action=edit Bug 33447: (follow-up) Fix tests and make assumption explicit The patches made an assumption that patron would always be passed. It is within Koha, but not in the Biblios tests. There is no scenario where we can determine pickup locations that are not in reference to a patron (who is picking it up?) so we should always have this parameter -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #149968|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 --- Comment #18 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 149981 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=149981&action=edit Bug 33447: (follow-up) Fix tests and make assumption explicit The patches made an assumption that patron would always be passed. It is within Koha, but not in the Biblios tests. There is no scenario where we can determine pickup locations that are not in reference to a patron (who is picking it up?) so we should always have this parameter -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 --- Comment #19 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 149982 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=149982&action=edit Bug 33447: Make *->pickup_locations methods throw an exception on missing parameter This patch fixes the FIXME for making the methods throw an exception. Tests are added, and POD is adapted as well. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/patrons* \ t/db_dependent/api/v1/holds.t \ t/db_dependent/Reserves* \ t/db_dependent/Hold* \ t/db_dependent/Koha/Hold* \ t/db_dependent/Items* => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Jacob O'Mara <jacob.omara@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable Version(s)|23.05.00 |23.05.00,22.11.06 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 --- Comment #20 from Jacob O'Mara <jacob.omara@ptfs-europe.com> --- Nice work, thanks everyone! Pushed to 22.11.x for the next release. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 --- Comment #21 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Jacob O'Mara from comment #20)
Nice work, thanks everyone!
Pushed to 22.11.x for the next release.
You broke Jenkins Can't locate Koha/Exceptions/Checkin.pm -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com Depends on| |32121 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32121 [Bug 32121] Show an alert when adding a checked out item to an item bundle -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Bug 33447 depends on bug 32121, which changed state. Bug 32121 Summary: Show an alert when adding a checked out item to an item bundle https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32121 What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Bug 33447 depends on bug 32121, which changed state. Bug 32121 Summary: Show an alert when adding a checked out item to an item bundle https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32121 What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |REOPENED Resolution|FIXED |--- -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Bug 33447 depends on bug 32121, which changed state. Bug 32121 Summary: Show an alert when adding a checked out item to an item bundle https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32121 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33447 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Resolution|--- |FIXED Status|Pushed to stable |RESOLVED --- Comment #22 from Lucas Gass <lucas@bywatersolutions.com> --- Missing dependencies for 22.05.x, no backport. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org