CanBookBeRenewed has a few performance issues: 1 - We call CheckReserves multiple times, however, a single call returns all possible reserves 2 - We fetch items and patrons one at a time, however, we can use a search to get all at once
Created attachment 118592 [details] [review] Bug 28013: Performance improvements to CanBookBeRenewed In the case of 'AllowRenewalIfOtherItemsAvailable' we check all existing reserves against all existing items. This patchset reduces the number of DB/subroutine calls To test: 1 - Apply patch 2 - prove -v t/db_dependent/Circulation.t
Created attachment 118928 [details] [review] Bug 28013: Performance improvements to CanBookBeRenewed To test: 1 - Apply patch 2 - prove -v t/db_dependent/Circulation.t
Created attachment 118950 [details] [review] Bug 28013: Performance improvements to CanBookBeRenewed In the case of 'AllowRenewalIfOtherItemsAvailable' we check all existing reserves against all existing items. This patchset reduces the number of DB/subroutine calls To test: 1 - Apply patch 2 - prove -v t/db_dependent/Circulation.t Signed-off-by: Amit Gupta <amitddng135@gmail.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 118951 [details] [review] Bug 28013: (QA follow-up) Remove unused variable Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
I want to check one more thing.. Moving this back to SO meanwhile. I wonder what happens with when there is two waiting holds and one checked out item.
Ok, that works as well. Passing QA. The new code is is more simple! :) Hopefully we can get rid of the remaining bugs in this function in other bug reports.
I don't manage to convince myself it's correct. There is an edge case, looking at the code: The patron iterator is set before the ITEM loop. On the second item, it won't be reset and we won't iterate over all the patrons. Nick and/or Joonas, can you double check it please?
Created attachment 119125 [details] [review] Bug 28013: (follow-up) Correct and update call to CanItemBeReserved The current call checks for truth against a returned hash, we need to check the 'status' value
Created attachment 119126 [details] [review] Bug 28013: Unit tests Cover the case of multiple patrons and multiple items that can fill them
Created attachment 119127 [details] [review] Bug 28013: (follow-up) Reset the patron loop To test: 1 - Apply all other patches and dependencies 2 - prove -v t/db_dependent/Circulation.t 3 - It fails 4 - Apply this patch 5 - It passes
(In reply to Jonathan Druart from comment #7) > I don't manage to convince myself it's correct. > There is an edge case, looking at the code: > > The patron iterator is set before the ITEM loop. On the second item, it > won't be reset and we won't iterate over all the patrons. > > Nick and/or Joonas, can you double check it please? Indeed, simply resetting after the loop should cover that. Adding test to prove that, however, uncovered additional problems. I filed bug 28078 and updated code here. I set to SO for another QA round to check what I added.
Nick, is it possible to drop the dependency to bug 28078? I mean if we don't take it would the behaviour change with your refactoring here or stay same? Please see my comments in bug 28078, it is probably not the right solution there so if we can drop that as dependency to this it would be great as the discussion over there wouldn't block this.
(In reply to Joonas Kylmälä from comment #12) > Nick, is it possible to drop the dependency to bug 28078? I mean if we don't > take it would the behaviour change with your refactoring here or stay same? > Please see my comments in bug 28078, it is probably not the right solution > there so if we can drop that as dependency to this it would be great as the > discussion over there wouldn't block this. If we only keep the last patch here and drop the correction of the call to CanItemBeReserved and the units tests we can. They can be filed as separate bugs if you think that is best
(In reply to Nick Clemens from comment #13) > (In reply to Joonas Kylmälä from comment #12) > > Nick, is it possible to drop the dependency to bug 28078? I mean if we don't > > take it would the behaviour change with your refactoring here or stay same? > > Please see my comments in bug 28078, it is probably not the right solution > > there so if we can drop that as dependency to this it would be great as the > > discussion over there wouldn't block this. > > If we only keep the last patch here and drop the correction of the call to > CanItemBeReserved and the units tests we can. > > They can be filed as separate bugs if you think that is best I think it is for the best. Isn't the bug already reported in: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659
Nick, I reviewed the bug 28078 again and your fix there seems great. I think we can keep the patches here like this, I will QA the whole thing after the minor bug I spotted in bug 28078 is fixed. Sounds good?
Moving back to signed-off until I am able to upload the patches with my sign-off (currently bugzilla is experiencing some difficulties)
Created attachment 119253 [details] [review] Bug 28013: Performance improvements to CanBookBeRenewed In the case of 'AllowRenewalIfOtherItemsAvailable' we check all existing reserves against all existing items. This patchset reduces the number of DB/subroutine calls To test: 1 - Apply patch 2 - prove -v t/db_dependent/Circulation.t Signed-off-by: Amit Gupta <amitddng135@gmail.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 119257 [details] [review] Bug 28013: Performance improvements to CanBookBeRenewed In the case of 'AllowRenewalIfOtherItemsAvailable' we check all existing reserves against all existing items. This patchset reduces the number of DB/subroutine calls To test: 1 - Apply patch 2 - prove -v t/db_dependent/Circulation.t Signed-off-by: Amit Gupta <amitddng135@gmail.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 119258 [details] [review] Bug 28013: (QA follow-up) Remove unused variable Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 119259 [details] [review] Bug 28013: (follow-up) Correct and update call to CanItemBeReserved The current call checks for truth against a returned hash, we need to check the 'status' value Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 119260 [details] [review] Bug 28013: Unit tests Cover the case of multiple patrons and multiple items that can fill them Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 119261 [details] [review] Bug 28013: (follow-up) Reset the patron loop To test: 1 - Apply all other patches and dependencies 2 - prove -v t/db_dependent/Circulation.t 3 - It fails 4 - Apply this patch 5 - It passes Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Passing QA.
Pushed to master for 21.05, thanks to everybody involved!
*** Bug 26659 has been marked as a duplicate of this bug. ***
Pushed to 20.11.x for 20.11.06
Pushed to 20.05.x for 20.05.12
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.