The background jobs queue will need purging of finished jobs. Bug 31629 focuses on the elastic indexing jobs but the table will fill up with other types of jobs
Created attachment 142603 [details] [review] Bug 31969 - Add options for cleanup_database.pl to remove finished jobs from the background job queue
Bug 31969: Add background queue options to cleanup_database This patch adds background queue options to cleanup_database.pl to allow for purging completed background jobs. --bg-jobs DAYS Purge all finished background jobs this many days old. Defaults to 1 if no DAYS provided. --bg-type TYPE What type of background job to purge. Defaults to "update_elastic_index" if omitted Specifying "all" will purge all types. Repeatable. To test: 1 - Enable elastic search in Koha 2 - perl misc/maintenance/touch_all_items.pl 3 - Generate an number of diffrent types of bg-jobs (eg batch_hold_cancel, batch_biblio_record_deletion, batch_item_record_deletion) 4 - Check db and note there are a bunch of diffrent jobs 5 - Update to make them old UPDATE background_jobs SET enqued_on = '2022-10-01 00:00:00', status='finished' 6 - perl misc/cronjobs/cleanup_database.pl 7 - Note bg-jobs entry shows in help 8 - perl misc/cronjobs/cleanup_database.pl --bg-jobs 1 -v 9 - Note that elasticqueue would have been cleared 10 - perl misc/cronjobs/cleanup_database.pl --bg-jobs 1 -v --confirm 11 - Note that number of entries deleted is reported 12 - Attempt to clear other job types, including "all" eg perl misc/cronjobs/cleanup_database.pl --bg-jobs 1 --bg-type batch_item_record_deletion -v --confirm perl misc/cronjobs/cleanup_database.pl --bg-jobs 1 --bg-type all -v --confirm 13 - Confirm in staff interface that jobs are gone: http://localhost:8081/cgi-bin/koha/admin/background_jobs.pl (Uncheck 'Current jobs only')
*** Bug 31970 has been marked as a duplicate of this bug. ***
*** Bug 31978 has been marked as a duplicate of this bug. ***
YES thank you
Thanks Björn for your patch. Have you considered putting the code of your PurgeBackgroundJobs function into Koha::BackgroundJobs as a class method? This could be used from elsewhere, the WUI for example, and from cleanup_database.pl like that: Koha::BackgroundJobs::purge(10, 'all', 1);
Created attachment 142668 [details] [review] Bug 31969: Add background queue options to cleanup_database This patch adds background queue options to cleanup_database.pl to allow for purging completed background jobs. --bg-jobs DAYS Purge all finished background jobs this many days old. Defaults to 1 if no DAYS provided. --bg-type TYPE What type of background job to purge. Defaults to "update_elastic_index" if omitted Specifying "all" will purge all types. Repeatable. To test: 1 - Enable elastic search in Koha 2 - perl misc/maintenance/touch_all_items.pl 3 - Generate an number of diffrent types of bg-jobs (eg batch_hold_cancel, batch_biblio_record_deletion, batch_item_record_deletion) 4 - Check db and note there are a bunch of diffrent jobs 5 - Update to make them old UPDATE background_jobs SET ended_on = '2022-10-01 00:00:00', status='finished' 6 - perl misc/cronjobs/cleanup_database.pl 7 - Note bg-jobs entry shows in help 8 - perl misc/cronjobs/cleanup_database.pl --bg-jobs 1 -v 9 - Note that elasticqueue would have been cleared 10 - perl misc/cronjobs/cleanup_database.pl --bg-jobs 1 -v --confirm 11 - Note that number of entries deleted is reported 12 - Attempt to clear other job types, including "all" eg perl misc/cronjobs/cleanup_database.pl --bg-jobs 1 --bg-type batch_item_record_deletion -v --confirm perl misc/cronjobs/cleanup_database.pl --bg-jobs 1 --bg-type all -v --confirm 13 - Confirm in staff interface that jobs are gone: http://localhost:8081/cgi-bin/koha/admin/background_jobs.pl (Uncheck 'Current jobs only') Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to Frédéric Demians from comment #6) > Thanks Björn for your patch. Have you considered putting the code of your > PurgeBackgroundJobs function into Koha::BackgroundJobs as a class method? > This > could be used from elsewhere, the WUI for example, and from > cleanup_database.pl > like that: > > Koha::BackgroundJobs::purge(10, 'all', 1); Might be an idea. Easy to do but writing the needed tests is a bit of a pain ( for me at least :) ).
Created attachment 142699 [details] [review] [Follow-up] Move deletion code to new method Koha::BackgroundJobs->purge Follow-up: Move deletion to new method Koha::BackgroundJobs->purge for reuse in other places. Functionality unchanged.
Created attachment 142700 [details] [review] [Follow-up] Test for Koha::BackgroundJobs->purge [Follow-up] Test for Koha::BackgroundJobs->purge prove -v t/db_dependent/Koha/BackgroundJobs.t should pass. Feel free to accept this here or if it should be in a seperate bug?
Thx Björn!
Created attachment 142727 [details] [review] Bug 31969: Add background queue options to cleanup_database This patch adds background queue options to cleanup_database.pl to allow for purging completed background jobs. --bg-jobs DAYS Purge all finished background jobs this many days old. Defaults to 1 if no DAYS provided. --bg-type TYPE What type of background job to purge. Defaults to "update_elastic_index" if omitted Specifying "all" will purge all types. Repeatable. To test: 1 - Enable elastic search in Koha 2 - perl misc/maintenance/touch_all_items.pl 3 - Generate an number of diffrent types of bg-jobs (eg batch_hold_cancel, batch_biblio_record_deletion, batch_item_record_deletion) 4 - Check db and note there are a bunch of diffrent jobs 5 - Update to make them old UPDATE background_jobs SET ended_on = '2022-10-01 00:00:00', status='finished' 6 - perl misc/cronjobs/cleanup_database.pl 7 - Note bg-jobs entry shows in help 8 - perl misc/cronjobs/cleanup_database.pl --bg-jobs 1 -v 9 - Note that elasticqueue would have been cleared 10 - perl misc/cronjobs/cleanup_database.pl --bg-jobs 1 -v --confirm 11 - Note that number of entries deleted is reported 12 - Attempt to clear other job types, including "all" eg perl misc/cronjobs/cleanup_database.pl --bg-jobs 1 --bg-type batch_item_record_deletion -v --confirm perl misc/cronjobs/cleanup_database.pl --bg-jobs 1 --bg-type all -v --confirm 13 - Confirm in staff interface that jobs are gone: http://localhost:8081/cgi-bin/koha/admin/background_jobs.pl (Uncheck 'Current jobs only') Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 142728 [details] [review] Bug 31969: (follow-up) Move deletion code to Koha::BackgroundJobs->purge for reuse. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 142729 [details] [review] Bug 31969: (follow-up) Unit tests for Koha::BackgroundJobs->purge Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 22.11. Nice work everyone, thanks!
*** Bug 31629 has been marked as a duplicate of this bug. ***
Any good reasons to not have used the filter_by_last_update methods for filtering, which is the pattern used all around cleanup_database?
Since when are we sending a confirm flag to a method? Why isn't this simply reusing the same pattern that exist for other flags? 381 if ($pMessages) { 382 print "Purging messages older than $pMessages days.\n" if $verbose; 383 my $messages = Koha::Patron::Messages->filter_by_last_update( 384 { timestamp_column_name => 'message_date', days => $pMessages } ); 385 my $count = $messages->count; 386 $messages->delete if $confirm; 387 if ( $verbose ) { 388 say $confirm 389 ? sprintf( "Done with purging %d messages", $count ) 390 : sprintf( "%d messages would have been removed", $count ); 391 } 392 } That's all what you needed. No additional methods, unit tests, etc.
Also, before this is released in 22.11, should not we replace "background" and "bg" with "job"?
Created attachment 144189 [details] [review] Bug 31969: Use filter_by_last_update This script has a pattern to delete rows depending on a given date/number of days, we should use the filter_by_last_update Koha::Objects method. No need for another method and tests, everything is already tested there. This patch also suggests to rename the reference to "background" and "bg" with "jobs", which seems more appropriate and not an abbreviation
Created attachment 144190 [details] [review] Bug 31969: Use filter_by_last_update This script has a pattern to delete rows depending on a given date/number of days, we should use the filter_by_last_update Koha::Objects method. No need for another method and tests, everything is already tested there. This patch also suggests to rename the reference to "background" and "bg" with "jobs", which seems more appropriate and not an abbreviation
(In reply to Jonathan Druart from comment #21) > Created attachment 144190 [details] [review] [review] > Bug 31969: Use filter_by_last_update > > This script has a pattern to delete rows depending on a given > date/number of days, we should use the filter_by_last_update > Koha::Objects method. > No need for another method and tests, everything is already tested > there. > > This patch also suggests to rename the reference to "background" and > "bg" with "jobs", which seems more appropriate and not an abbreviation Can we move this to a different bug? This has already been released so I think we need to tackle it from a new bug now.
(In reply to Katrin Fischer from comment #22) > (In reply to Jonathan Druart from comment #21) > > Created attachment 144190 [details] [review] [review] [review] > > Bug 31969: Use filter_by_last_update > > > > This script has a pattern to delete rows depending on a given > > date/number of days, we should use the filter_by_last_update > > Koha::Objects method. > > No need for another method and tests, everything is already tested > > there. > > > > This patch also suggests to rename the reference to "background" and > > "bg" with "jobs", which seems more appropriate and not an abbreviation > > Can we move this to a different bug? This has already been released so I > think we need to tackle it from a new bug now. The patch has been pushed to master. commit 404c88fe87b27e4b4578e0bb6df2252a0bad9ed1