The autorenewals cron script is in effect doing a bulk renew action.. but right now it iterates through all renewals and calls 'AddRenewal' on each item individually.. This can cause things, like elastic indexing, to queue up but without actioning it via the skip_index option. We aught to be able to pass skip_indexing to AddRenewal to allow us to action the indexing step at the end as a bulk change for the ES case.
As a temporary fix I've replaced the throw with a warn in Koha/SearchEngine/Elasticsearch/Indexer.pm so that we fail more gracefully rather than dieing and killing the autorenewals script part way through. This will of course mean the item in question isn't properly indexed.. but at least the rest of the process will run to completion.
Created attachment 144442 [details] [review] Bug 32013: Autorenewal batch indexing The automatic_renewals.pl cron script currently loops through items for automatic renewal and calls the indexer for each one individually. skip_record_index has now been added as a parameter to the AddRenewal function to skip the indexing process. The item numbers are now added to an array and then the indexer is called once from within automatic_renewals.pl and passed the array to queue one indexing job instead of multiple jobs. Test plan: 1) AddRenewal uses Koha::Items->store() to trigger the indexing process. Run prove -vv t/db_dependent/Koha/SearchEngine/Indexer.t and check tests 5,6,29,30. These tests prove whether passing skip_record_index to store() triggers or skips the indexing process. All four tests should pass to show that skip_index_records can prevent the indexing being triggered. 2) Add multiple renewals that are able to be autorenewed and run the automatic_renewals.pl script. There should be multiple items queued in zebraqueue. 3) Apply patch and try again 4) There should now only be one job queued in zebraqueue
Created attachment 144464 [details] [review] Bug 32013: (follow-up) Add unit test A unit test has been added to show the pass-through of skip_record_index from AddRenewal() to the Item->store() method. Test plan: 1) In the shell run prove -vv t/db_dependent/Koha/SearchEngine/Indexer.t 2) Observe Subtest 3: Test AddRenewal indexer call 3) All four tests should pass
Created attachment 145150 [details] [review] Bug 32013: Autorenewal batch indexing The automatic_renewals.pl cron script currently loops through items for automatic renewal and calls the indexer for each one individually. skip_record_index has now been added as a parameter to the AddRenewal function to skip the indexing process. The item numbers are now added to an array and then the indexer is called once from within automatic_renewals.pl and passed the array to queue one indexing job instead of multiple jobs. Test plan: 1) AddRenewal uses Koha::Items->store() to trigger the indexing process. Run prove -vv t/db_dependent/Koha/SearchEngine/Indexer.t and check tests 5,6,29,30. These tests prove whether passing skip_record_index to store() triggers or skips the indexing process. All four tests should pass to show that skip_index_records can prevent the indexing being triggered. 2) Add multiple renewals that are able to be autorenewed and run the automatic_renewals.pl script. There should be multiple items queued in zebraqueue. 3) Apply patch and try again 4) There should now only be one job queued in zebraqueue Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 145151 [details] [review] Bug 32013: (follow-up) Add unit test A unit test has been added to show the pass-through of skip_record_index from AddRenewal() to the Item->store() method. Test plan: 1) In the shell run prove -vv t/db_dependent/Koha/SearchEngine/Indexer.t 2) Observe Subtest 3: Test AddRenewal indexer call 3) All four tests should pass Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 145152 [details] [review] Bug 32013: (QA follow-up) Move mock to avoid warns from sample data creation Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 145186 [details] [review] Bug 32013: Autorenewal batch indexing The automatic_renewals.pl cron script currently loops through items for automatic renewal and calls the indexer for each one individually. skip_record_index has now been added as a parameter to the AddRenewal function to skip the indexing process. The item numbers are now added to an array and then the indexer is called once from within automatic_renewals.pl and passed the array to queue one indexing job instead of multiple jobs. Test plan: 1) AddRenewal uses Koha::Items->store() to trigger the indexing process. Run prove -vv t/db_dependent/Koha/SearchEngine/Indexer.t and check tests 5,6,29,30. These tests prove whether passing skip_record_index to store() triggers or skips the indexing process. All four tests should pass to show that skip_index_records can prevent the indexing being triggered. 2) Add multiple renewals that are able to be autorenewed and run the automatic_renewals.pl script. There should be multiple items queued in zebraqueue. 3) Apply patch and try again 4) There should now only be one job queued in zebraqueue Mentored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 145187 [details] [review] Bug 32013: (follow-up) Add unit test A unit test has been added to show the pass-through of skip_record_index from AddRenewal() to the Item->store() method. Test plan: 1) In the shell run prove -vv t/db_dependent/Koha/SearchEngine/Indexer.t 2) Observe Subtest 3: Test AddRenewal indexer call 3) All four tests should pass Mentored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 145188 [details] [review] Bug 32013: (QA follow-up) Move mock to avoid warns from sample data creation Mentored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Added my mentoring lines whilst doing a double check of the code.. I'm really happy with this improvement, thanks Matt. Thanks Nick for the follow-up and review.
Minor patch, moving to PQA
Please rebase ASAP.
Looks like a conflict with bug 33309 which is avoiding a race condition - this would still call the indexer on each call to AddRenewal rather than skipping the individual indexing jobs and running one indexing job on the collected biblio ids. I don't think I can rebase in a way that doesn't involve deleting the intent of bug 33309 - looks like Nick wrote that bug and should be on the thread for this as he signed it off. Nick - Does this patch cover what you were trying to achieve with 33309 or do we need to combine them in some way?
Created attachment 149276 [details] [review] Bug 32013: Autorenewal batch indexing The automatic_renewals.pl cron script currently loops through items for automatic renewal and calls the indexer for each one individually. skip_record_index has now been added as a parameter to the AddRenewal function to skip the indexing process. The item numbers are now added to an array and then the indexer is called once from within automatic_renewals.pl and passed the array to queue one indexing job instead of multiple jobs. Test plan: 1) AddRenewal uses Koha::Items->store() to trigger the indexing process. Run prove -vv t/db_dependent/Koha/SearchEngine/Indexer.t and check tests 5,6,29,30. These tests prove whether passing skip_record_index to store() triggers or skips the indexing process. All four tests should pass to show that skip_index_records can prevent the indexing being triggered. 2) Add multiple renewals that are able to be autorenewed and run the automatic_renewals.pl script. There should be multiple items queued in zebraqueue. 3) Apply patch and try again 4) There should now only be one job queued in zebraqueue Mentored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 149277 [details] [review] Bug 32013: (follow-up) Add unit test A unit test has been added to show the pass-through of skip_record_index from AddRenewal() to the Item->store() method. Test plan: 1) In the shell run prove -vv t/db_dependent/Koha/SearchEngine/Indexer.t 2) Observe Subtest 3: Test AddRenewal indexer call 3) All four tests should pass Mentored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 149278 [details] [review] Bug 32013: (QA follow-up) Move mock to avoid warns from sample data creation Mentored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 149279 [details] [review] Bug 32013: Adjust unit tests
(In reply to Matt Blenkinsop from comment #13) > Nick - Does this patch cover what you were trying to achieve with 33309 or > do we need to combine them in some way? I think leaving the check I added, and moving your check down to the new section afterward seems to work - can you and Martin give my updates a once over?
Seems logical to me and code looks fine - thanks Nick!
Pushed to master for 23.05. Nice work everyone, thanks!
Enhancement - not backporting to 22.11.x Nice work everyone!
No need for any changes in the Manual; updating status from Needs documenting to Resolved.