Bug 31969 - Options for cleanup_database.pl to remove finished jobs from the background job queue
Summary: Options for cleanup_database.pl to remove finished jobs from the background j...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Björn Nylén
QA Contact: Testopia
URL:
Keywords:
: 31629 31970 31978 (view as bug list)
Depends on:
Blocks: 35948 32093 32304
  Show dependency treegraph
 
Reported: 2022-10-25 10:16 UTC by Björn Nylén
Modified: 2024-01-30 14:30 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 31969 - Add options for cleanup_database.pl to remove finished jobs from the background job queue (7.44 KB, patch)
2022-10-25 10:19 UTC, Björn Nylén
Details | Diff | Splinter Review
Bug 31969: Add background queue options to cleanup_database (7.49 KB, patch)
2022-10-26 12:31 UTC, Nick Clemens
Details | Diff | Splinter Review
[Follow-up] Move deletion code to new method Koha::BackgroundJobs->purge (3.66 KB, patch)
2022-10-27 09:25 UTC, Björn Nylén
Details | Diff | Splinter Review
[Follow-up] Test for Koha::BackgroundJobs->purge (3.58 KB, patch)
2022-10-27 09:28 UTC, Björn Nylén
Details | Diff | Splinter Review
Bug 31969: Add background queue options to cleanup_database (7.54 KB, patch)
2022-10-27 18:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31969: (follow-up) Move deletion code to Koha::BackgroundJobs->purge for reuse. (3.71 KB, patch)
2022-10-27 18:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31969: (follow-up) Unit tests for Koha::BackgroundJobs->purge (3.63 KB, patch)
2022-10-27 18:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31969: Use filter_by_last_update (10.54 KB, patch)
2022-11-23 08:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31969: Use filter_by_last_update (10.52 KB, patch)
2022-11-23 09:08 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Björn Nylén 2022-10-25 10:16:40 UTC
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
Comment 1 Björn Nylén 2022-10-25 10:19:31 UTC
Created attachment 142603 [details] [review]
Bug 31969 - Add options for cleanup_database.pl to remove finished jobs from the background job queue
Comment 2 Björn Nylén 2022-10-25 10:20:36 UTC
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')
Comment 3 Katrin Fischer 2022-10-25 11:32:33 UTC
*** Bug 31970 has been marked as a duplicate of this bug. ***
Comment 4 Katrin Fischer 2022-10-25 21:09:48 UTC
*** Bug 31978 has been marked as a duplicate of this bug. ***
Comment 5 Liz Rea 2022-10-25 22:46:28 UTC
YES thank you
Comment 6 Frédéric Demians 2022-10-26 06:20:54 UTC
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);
Comment 7 Nick Clemens 2022-10-26 12:31:58 UTC
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>
Comment 8 Björn Nylén 2022-10-26 12:40:23 UTC
(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 :) ).
Comment 9 Björn Nylén 2022-10-27 09:25:00 UTC
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.
Comment 10 Björn Nylén 2022-10-27 09:28:28 UTC
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?
Comment 11 Katrin Fischer 2022-10-27 17:56:45 UTC
Thx Björn!
Comment 12 Tomás Cohen Arazi 2022-10-27 18:00:30 UTC
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>
Comment 13 Tomás Cohen Arazi 2022-10-27 18:00:39 UTC
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>
Comment 14 Tomás Cohen Arazi 2022-10-27 18:00:46 UTC
Created attachment 142729 [details] [review]
Bug 31969: (follow-up) Unit tests for Koha::BackgroundJobs->purge

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2022-10-27 18:05:19 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 16 Nick Clemens 2022-10-28 14:30:43 UTC
*** Bug 31629 has been marked as a duplicate of this bug. ***
Comment 17 Jonathan Druart 2022-11-21 12:23:52 UTC
Any good reasons to not have used the filter_by_last_update methods for filtering, which is the pattern used all around cleanup_database?
Comment 18 Jonathan Druart 2022-11-21 12:32:05 UTC
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.
Comment 19 Jonathan Druart 2022-11-23 08:33:15 UTC
Also, before this is released in 22.11, should not we replace "background" and "bg" with "job"?
Comment 20 Jonathan Druart 2022-11-23 08:59:19 UTC
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
Comment 21 Jonathan Druart 2022-11-23 09:08:16 UTC
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
Comment 22 Katrin Fischer 2022-12-04 12:55:46 UTC
(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.
Comment 23 Jonathan Druart 2022-12-05 13:37:05 UTC
(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