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
Created attachment 149295 [details] [review] 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
Created attachment 149296 [details] [review] Bug 33447: Adjust tests
Created attachment 149389 [details] [review] 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>
Created attachment 149390 [details] [review] Bug 33447: Adjust tests Signed-off-by: emlam <emily.lamancusa@montgomerycountymd.gov>
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.
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?
(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.
(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
(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.
(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..
(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
(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.
Created attachment 149790 [details] [review] 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>
Created attachment 149791 [details] [review] Bug 33447: Adjust tests Signed-off-by: emlam <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 149792 [details] [review] Bug 33447: (QA follow-up) Add comment for flushing cache Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed to master for 23.05. Nice work everyone, thanks!
Created attachment 149968 [details] [review] 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
Created attachment 149981 [details] [review] 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
Created attachment 149982 [details] [review] 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>
Nice work, thanks everyone! Pushed to 22.11.x for the next release.
(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
Missing dependencies for 22.05.x, no backport.