Bug 27783 - Introduce background job queues
Summary: Introduce background job queues
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Kyle M Hall
URL:
Keywords: release-notes-needed
: 30350 (view as bug list)
Depends on: 30345
Blocks: 30656 32394 27344 29346 30410 30889
  Show dependency treegraph
 
Reported: 2021-02-24 19:46 UTC by Martin Renvoize
Modified: 2022-12-02 14:59 UTC (History)
7 users (show)

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


Attachments
Bug 27783: Replace --job-type by --job-queue (6.43 KB, patch)
2022-03-30 14:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27783: Add missing atomicupdate file (1.23 KB, patch)
2022-03-30 14:43 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 27783: Replace --job-type by --queue (6.39 KB, patch)
2022-03-30 15:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27783: Add missing atomicupdate file (1.29 KB, patch)
2022-03-30 15:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27783: (follow-up) Add DB change to installer (1.04 KB, patch)
2022-03-30 15:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27783: Add --job-type to background_jobs_worker.pl (3.01 KB, patch)
2022-04-01 12:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27783: Replace --job-type by --queue (5.84 KB, patch)
2022-04-01 12:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27783: Add missing atomicupdate file (1.29 KB, patch)
2022-04-01 12:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27783: (follow-up) Add DB change to installer (1.04 KB, patch)
2022-04-01 12:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27783: Rename queues and adjust currently defined jobs (4.52 KB, patch)
2022-04-01 12:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27783: Add --job-type to background_jobs_worker.pl (3.01 KB, patch)
2022-04-02 10:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27783: Replace --job-type by --queue (5.73 KB, patch)
2022-04-02 10:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27783: Add missing atomicupdate file (1.29 KB, patch)
2022-04-02 10:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27783: (follow-up) Add DB change to installer (1.04 KB, patch)
2022-04-02 10:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27783: Rename queues and adjust currently defined jobs (4.57 KB, patch)
2022-04-02 10:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27783: (QA follow-up) Make dbrev idempotent (1.67 KB, patch)
2022-04-02 10:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27783: DBIC update [DO NOT PUSH] (1.68 KB, patch)
2022-04-04 17:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27783: Add queue handling in packaging tools (9.13 KB, patch)
2022-04-04 20:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27783: Add queue handling in packaging tools (10.36 KB, patch)
2022-04-04 20:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27783: Add --job-type to background_jobs_worker.pl (3.07 KB, patch)
2022-04-11 14:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27783: Add --job-type to background_jobs_worker.pl (3.07 KB, patch)
2022-04-11 14:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27783: Replace --job-type by --queue (5.79 KB, patch)
2022-04-11 14:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27783: Add missing atomicupdate file (1.35 KB, patch)
2022-04-11 14:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27783: (follow-up) Add DB change to installer (1.09 KB, patch)
2022-04-11 14:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27783: Rename queues and adjust currently defined jobs (4.62 KB, patch)
2022-04-11 14:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27783: (QA follow-up) Make dbrev idempotent (1.72 KB, patch)
2022-04-11 14:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27783: DBIC update [DO NOT PUSH] (1.74 KB, patch)
2022-04-11 14:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27783: Add queue handling in packaging tools (10.42 KB, patch)
2022-04-11 14:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27783: (QA follow-up) Make dbrev executable (610 bytes, patch)
2022-04-11 14:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27783: Add --job-type to background_jobs_worker.pl (3.13 KB, patch)
2022-04-11 15:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27783: Replace --job-type by --queue (5.85 KB, patch)
2022-04-11 15:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27783: Add missing atomicupdate file (1.41 KB, patch)
2022-04-11 15:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27783: (follow-up) Add DB change to installer (1.16 KB, patch)
2022-04-11 15:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27783: Rename queues and adjust currently defined jobs (4.68 KB, patch)
2022-04-11 15:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27783: (QA follow-up) Make dbrev idempotent (1.79 KB, patch)
2022-04-11 15:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27783: DBIC update [DO NOT PUSH] (1.80 KB, patch)
2022-04-11 15:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27783: Add queue handling in packaging tools (10.48 KB, patch)
2022-04-11 15:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27783: (QA follow-up) Make dbrev executable (675 bytes, patch)
2022-04-11 15:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27783: (follow-up) Rename options (3.04 KB, patch)
2022-04-11 15:14 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2021-02-24 19:46:18 UTC
Right now our worker will just pick tasks of the queue in a fifo order.. we should really allow the worker to be more specific and have different workers picking jobs from different queues.. and we shouldn't limit ourselves to 'one queue for job type' or 'one queue for all jobs types.
Comment 1 Martin Renvoize 2022-03-30 14:13:03 UTC
Created attachment 132642 [details] [review]
Bug 27783: Replace --job-type by --job-queue

This patch adds a new column background_jobs.queue, which default to
'default'
By default, new jobs are enqueued into this default queue, and the
background job worker will subscribe to the default queue unless told
otherwise by the --job-queue option

The new job UpdateElasticIndex is automatically enqueued in another
queue named 'index'
So you can have 'default' worker with
    misc/background_jobs_worker.pl
and a dedicated indexing worker with
    misc/background_jobs_worker.pl --job-queue index

This is to address bug 27344 comment #15

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 2 Julian Maurice 2022-03-30 14:43:29 UTC
Created attachment 132644 [details] [review]
Bug 27783: Add missing atomicupdate file
Comment 3 Martin Renvoize 2022-03-30 15:46:04 UTC
Created attachment 132649 [details] [review]
Bug 27783: Replace --job-type by --queue

This patch adds a new column background_jobs.queue, which default to
'default'
By default, new jobs are enqueued into this default queue, and the
background job worker will subscribe to the default queue unless told
otherwise by the --job-queue option

The new job UpdateElasticIndex is automatically enqueued in another
queue named 'index'
So you can have 'default' worker with
    misc/background_jobs_worker.pl
and a dedicated indexing worker with
    misc/background_jobs_worker.pl --queue index

This is to address bug 27344 comment #15

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2022-03-30 15:46:09 UTC
Created attachment 132650 [details] [review]
Bug 27783: Add missing atomicupdate file

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2022-03-30 15:46:12 UTC
Created attachment 132651 [details] [review]
Bug 27783: (follow-up) Add DB change to installer

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2022-03-30 15:50:33 UTC
OK.. This works, but...

We end up without a worker running under default installs that will run the indexing jobs.

We should either add another set of debian templates etc to create the new worker along side the existing koha.worker service...

Or.. and I think it is perhaps preferable.. we should work on a worker manager daemon script instead of a simple worker script.. such a script would for workers, plural, to run the various queues.
Comment 7 David Cook 2022-03-30 22:39:21 UTC
I think that a "default" queue is the wrong approach. 

Tasks should be in "to-purpose" queues, so that they can have "to-purpose" workers. 

That said, I don't think the "--job-type" style is sustainable/maintainable as it could get unmanageably long. 

I suggest we use configuration files. Out of the box, workers use "default_worker_conf.json" which contains all the queues (this file is maintained by the koha-common package). This can be customized using a CLI arg like "-c,--config /path/to/custom_worker_conf.json".

This way, Koha "just works" out of the box with no sysadmin effort. 

For more complex implementations, the onus is on sysadmins to update their custom worker configuration(s).
Comment 8 David Cook 2022-03-30 22:52:08 UTC
(In reply to Martin Renvoize from comment #6)
> Or.. and I think it is perhaps preferable.. we should work on a worker
> manager daemon script instead of a simple worker script.. such a script
> would for workers, plural, to run the various queues.

Good call. 

For other projects, I use systemd services or separate containers for workers, but with Koha we use the "daemon" utility to run our own services, so I think we need to do a bespoke manager as well...

I suppose koha-worker could consume a declarative configuration file that lists out how many workers to create and what configuration files they should use themselves. It would then invoke those using the "daemon" utility I suppose?
Comment 9 Tomás Cohen Arazi 2022-03-31 07:16:59 UTC
I was reading the code and thinking how plugin-generated tasks could fit here. And with the current approach, I was leaning into adding 'plugin-real-time' and 'plugin-long-running' queues on top of this. But the need to have 4 running workers sounds like too much...

What about having:

* 'real-time'
* 'turtle'

It felt like it would be enough. From the plugin standpoint (to be worked on bug 30410), I would add high-level methods for queueing real-time and long-running tasks, and it would be up to the plugin author to call the right one.
Comment 10 Tomás Cohen Arazi 2022-03-31 07:34:22 UTC
The 'turtle' queue name was put to make it obvious what I meant.
But it is cool too <.<
Comment 11 David Cook 2022-04-01 02:46:11 UTC
(In reply to Tomás Cohen Arazi from comment #10)
> The 'turtle' queue name was put to make it obvious what I meant.
> But it is cool too <.<

I laughed. I like it haha.

(In reply to Tomás Cohen Arazi from comment #9)
> I was reading the code and thinking how plugin-generated tasks could fit
> here. And with the current approach, I was leaning into adding
> 'plugin-real-time' and 'plugin-long-running' queues on top of this. But the
> need to have 4 running workers sounds like too much...

You wouldn't need to have 4 running workers though, since a worker can subscribe to multiple queues. 

Regardless of the implementation, I am keen to get plugin-generated tasks into Koha.

> What about having:
> 
> * 'real-time'
> * 'turtle'

Personally, I would prefer more granularity and configurability, but this does sound like a pragmatic compromise. 

Another benefit of having "real-time" and "turtle" would be that adding more background tasks should have a lower barrier to entry, since you'd just need to provide a class for the worker to use and an interface for enqueuing tasks. (I've been concerned that people haven't been taking advantage of the background tasks because they weren't sure how to work with it.)
Comment 12 David Cook 2022-04-01 02:47:56 UTC
(In reply to David Cook from comment #11)
> Another benefit of having "real-time" and "turtle" would be that adding more
> background tasks should have a lower barrier to entry, since you'd just need
> to provide a class for the worker to use and an interface for enqueuing
> tasks. (I've been concerned that people haven't been taking advantage of the
> background tasks because they weren't sure how to work with it.)

Admittedly, I've wanted to use the background tasks for local customizations, but the current amount of customization required of core Koha::BackgroundJob code has made me avoid it.
Comment 13 Tomás Cohen Arazi 2022-04-01 07:24:52 UTC
(In reply to David Cook from comment #11)
> 
> You wouldn't need to have 4 running workers though, since a worker can
> subscribe to multiple queues. 

Well, that's the point. You need to set special workers using the .pl script instead of koha-worker, etc... so it complicates things. This is what I valued the most when looking at this patchset.

> Another benefit of having "real-time" and "turtle" would be that adding more
> background tasks should have a lower barrier to entry, since you'd just need
> to provide a class for the worker to use and an interface for enqueuing
> tasks. (I've been concerned that people haven't been taking advantage of the
> background tasks because they weren't sure how to work with it.)

Look at the follow-up bugs and bug 30360 for ideas.
Comment 14 Tomás Cohen Arazi 2022-04-01 12:03:19 UTC
Created attachment 132846 [details] [review]
Bug 27783: Add --job-type to background_jobs_worker.pl

This will let sysop adjust the number of workers and how they want to
manage them.
For instance one could want to have one worker for ES indexation and
another worker for other jobs, to prevent ES index to be stuck behind
bigger batch process.

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2022-04-01 12:03:25 UTC
Created attachment 132847 [details] [review]
Bug 27783: Replace --job-type by --queue

This patch adds a new column background_jobs.queue, which default to
'default'
By default, new jobs are enqueued into this default queue, and the
background job worker will subscribe to the default queue unless told
otherwise by the --job-queue option

The new job UpdateElasticIndex is automatically enqueued in another
queue named 'index'
So you can have 'default' worker with
    misc/background_jobs_worker.pl
and a dedicated indexing worker with
    misc/background_jobs_worker.pl --queue index

This is to address bug 27344 comment #15

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Tomás Cohen Arazi 2022-04-01 12:03:32 UTC
Created attachment 132848 [details] [review]
Bug 27783: Add missing atomicupdate file

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Tomás Cohen Arazi 2022-04-01 12:03:37 UTC
Created attachment 132849 [details] [review]
Bug 27783: (follow-up) Add DB change to installer

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Tomás Cohen Arazi 2022-04-01 12:03:42 UTC
Created attachment 132850 [details] [review]
Bug 27783: Rename queues and adjust currently defined jobs

This patch renames the queues so the default is the **real-time** one, and
the other (*turtle*) is kept for **slow** tasks.

All current *batch* tasks are explicitly assigned to the **turtle**
queue as well.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Tomás Cohen Arazi 2022-04-01 12:06:27 UTC
Ok, after several talks at the hackfest, everyone agreed we should (or not opposed to) reverse the dependency to keep things clearer. I took on the task of rebasing, reorganizing things, etc keeping the attribution of the original patches.

To put it clear, this *should* only be lacking what it already lacked of: integration with koha-worker so both worker processes are launched by default to .deb install users.
Comment 20 Tomás Cohen Arazi 2022-04-02 10:49:22 UTC
Created attachment 132875 [details] [review]
Bug 27783: Add --job-type to background_jobs_worker.pl

This will let sysop adjust the number of workers and how they want to
manage them.
For instance one could want to have one worker for ES indexation and
another worker for other jobs, to prevent ES index to be stuck behind
bigger batch process.

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi 2022-04-02 10:49:27 UTC
Created attachment 132876 [details] [review]
Bug 27783: Replace --job-type by --queue

This patch adds a new column background_jobs.queue, which default to
'default'
By default, new jobs are enqueued into this default queue, and the
background job worker will subscribe to the default queue unless told
otherwise by the --job-queue option

The new job UpdateElasticIndex is automatically enqueued in another
queue named 'index'
So you can have 'default' worker with
    misc/background_jobs_worker.pl
and a dedicated indexing worker with
    misc/background_jobs_worker.pl --queue index

This is to address bug 27344 comment #15

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Tomás Cohen Arazi 2022-04-02 10:49:31 UTC
Created attachment 132877 [details] [review]
Bug 27783: Add missing atomicupdate file

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Tomás Cohen Arazi 2022-04-02 10:49:36 UTC
Created attachment 132878 [details] [review]
Bug 27783: (follow-up) Add DB change to installer

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Tomás Cohen Arazi 2022-04-02 10:49:41 UTC
Created attachment 132879 [details] [review]
Bug 27783: Rename queues and adjust currently defined jobs

This patch renames the queues so the default is the **real-time** one, and
the other (*long_tasks*) is kept for **long running** tasks.

All current *batch* tasks are explicitly assigned to the **long_tasks**
queue as well.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Tomás Cohen Arazi 2022-04-02 10:51:35 UTC
Somehow, my working branch was missing 30172, so I rebased this patches on top of it.
Comment 26 Tomás Cohen Arazi 2022-04-02 10:58:43 UTC
Created attachment 132880 [details] [review]
Bug 27783: (QA follow-up) Make dbrev idempotent

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Tomás Cohen Arazi 2022-04-04 17:51:37 UTC
Created attachment 132948 [details] [review]
Bug 27783: DBIC update [DO NOT PUSH]

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Tomás Cohen Arazi 2022-04-04 20:16:10 UTC
Created attachment 132950 [details] [review]
Bug 27783: Add queue handling in packaging tools

This patch adds queue name handling in is_worker_running and a handy
method to centralize process name handling for queue-specific workers.

To test:
1. Apply this patch
2. Run:
   $ source debian/scripts/koha-functions.sh
   $ get_worker_name kohadev
   kohadev-koha-worker
=> SUCCESS: Default queue means no queue name included
   $ get_worker_name kohadev default
   kohadev-koha-worker
=> SUCCESS: Default queue means no queue name included
   $ get_worker_name kohadev long_tasks
   kohadev-koha-worker-long_tasks
=> SUCCESS: Queue name appended to the name
4. Run:
   $ perl misc4dev/cp_debian_files.pl \
              --koha_dir /kohadevbox/koha \
              --gitify /kohadevbox/gitify \
              --instance kohadev
5. Run:
   $ koha-worker --start --queue oleonard kohadev
=> SUCCESS: Starts!
6. Check:
   $ ps waux | grep oleonard
=> SUCCESS: It is running
7. Play with --stop, --restart, --status and different queue names,
   including 'default' and without the --queue param as well.
=> SUCCESS: All as expected!
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 Tomás Cohen Arazi 2022-04-04 20:22:59 UTC
Created attachment 132951 [details] [review]
Bug 27783: Add queue handling in packaging tools

This patch adds queue name handling in is_worker_running and a handy
method to centralize process name handling for queue-specific workers.

To test:
1. Apply this patch
2. Run:
   $ source debian/scripts/koha-functions.sh
   $ get_worker_name kohadev
   kohadev-koha-worker
=> SUCCESS: Default queue means no queue name included
   $ get_worker_name kohadev default
   kohadev-koha-worker
=> SUCCESS: Default queue means no queue name included
   $ get_worker_name kohadev long_tasks
   kohadev-koha-worker-long_tasks
=> SUCCESS: Queue name appended to the name
4. Run:
   $ perl misc4dev/cp_debian_files.pl \
              --koha_dir /kohadevbox/koha \
              --gitify /kohadevbox/gitify \
              --instance kohadev
5. Run:
   $ koha-worker --start --queue oleonard kohadev
=> SUCCESS: Starts!
6. Check:
   $ ps waux | grep oleonard
=> SUCCESS: It is running
7. Play with --stop, --restart, --status and different queue names,
   including 'default' and without the --queue param as well.
=> SUCCESS: All as expected!
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Jonathan Druart 2022-04-05 12:56:25 UTC
1. Shouldn't we force the job to define its queue? Otherwise I have the feeling that we will end up with "default" everywhere.
2. What could be other queues?
3. Is BatchCancelHold really a long task? How do you define a short/long task then?
4. Do we really want long/short task distinction? If you index 1000 records (using batch update), it will be considered as "default" (/not long task), but it will take a while.
Comment 31 Tomás Cohen Arazi 2022-04-05 13:29:35 UTC
(In reply to Jonathan Druart from comment #30)
> 1. Shouldn't we force the job to define its queue? Otherwise I have the
> feeling that we will end up with "default" everywhere.

I think this can be caught at QA time. See 4.

> 2. What could be other queues?

Chatter on this and bug 27344 pointed to devs wantings some flexibility to run their own queues. That's why I reorganized things, keeping that in mind. Maybe later someone adds a configuration page/file to point specific tasks to specific queues, in order to offload some tasks to a separate physical server.

> 3. Is BatchCancelHold really a long task? How do you define a short/long
> task then?

I think that was a mistake I made. i.e. cancelling holds shouldn't wait for a long running batch import to finish.

> 4. Do we really want long/short task distinction? If you index 1000 records
> (using batch update), it will be considered as "default" (/not long task),
> but it will take a while.

We do want that. And maybe in the future we can add more queues... and priorities.
I think a follow-up is needed in bug 27344 to make it resort to long_tasks if the record count is high (for example, because of a batch operation) and be real-time if we are just updating an item index because it's been checked-in.
Comment 32 Jonathan Druart 2022-04-05 13:46:57 UTC
(In reply to Tomás Cohen Arazi from comment #31)
> (In reply to Jonathan Druart from comment #30)
> > 1. Shouldn't we force the job to define its queue? Otherwise I have the
> > feeling that we will end up with "default" everywhere.
> 
> I think this can be caught at QA time. See 4.

We have job_type that is doing that already.
Why asking a human to catch something that can be caught by a machine? :)

170 sub job_type { croak "This method must be subclassed" }
Comment 33 Jonathan Druart 2022-04-05 13:51:04 UTC
I am not sure I am understanding what bring 'queue' that cannot be done with the job_type.

We could have --job-type and a negation for this option.

perl background_jobs_worker.pl --job-type update_elastic_index
=> Run only indexation tasks

perl background_jobs_worker.pl --not-job-type update_elastic_index
=> Run all but indexation tasks
Comment 34 David Cook 2022-04-05 23:23:59 UTC
(In reply to Jonathan Druart from comment #33)
> I am not sure I am understanding what bring 'queue' that cannot be done with
> the job_type.
> 
> We could have --job-type and a negation for this option.
> 
> perl background_jobs_worker.pl --job-type update_elastic_index
> => Run only indexation tasks
> 
> perl background_jobs_worker.pl --not-job-type update_elastic_index
> => Run all but indexation tasks

I think that would get really messy/confusing as the number of job types/queues grows.

I still think pointing background_jobs_worker.pl at a configuration file makes the most sense.
Comment 35 Tomás Cohen Arazi 2022-04-06 08:31:32 UTC
(In reply to David Cook from comment #34)
> (In reply to Jonathan Druart from comment #33)
> > I am not sure I am understanding what bring 'queue' that cannot be done with
> > the job_type.
> > 
> > We could have --job-type and a negation for this option.
> > 
> > perl background_jobs_worker.pl --job-type update_elastic_index
> > => Run only indexation tasks
> > 
> > perl background_jobs_worker.pl --not-job-type update_elastic_index
> > => Run all but indexation tasks
> 
> I think that would get really messy/confusing as the number of job
> types/queues grows.
> 
> I still think pointing background_jobs_worker.pl at a configuration file
> makes the most sense.

Right, as the job types grows, and we don't even have control of them (plugins) we need some other way of grouping them more generally. Otherwise we cannot do cool things like installing a plugin and it will just work. We would need to set a worker for that. I'm sure you will insist on the ---not use for that, but think how we would solve the real-time vs. no hurry use cases in a generic way. It is plain straight-forward to just 'subscribe to a queue' for authors.
Comment 36 Tomás Cohen Arazi 2022-04-06 08:41:15 UTC
(In reply to Jonathan Druart from comment #30)
> 1. Shouldn't we force the job to define its queue? Otherwise I have the
> feeling that we will end up with "default" everywhere.

I've been trying to think of use cases, and the authority merge came to my mind. If the record count is low enough it would be great to do it real-time (because librarians really want to keep working on those records, probably), and if it is too big, it would be great to do it in the long_tasks queue (because it wouldn't need to wait for the cronjob). So, in this case, ->enqueue could actually put the job in different queues depending on the scenario.
Comment 37 Kyle M Hall 2022-04-11 14:14:49 UTC
Created attachment 133160 [details] [review]
Bug 27783: Add --job-type to background_jobs_worker.pl

This will let sysop adjust the number of workers and how they want to
manage them.
For instance one could want to have one worker for ES indexation and
another worker for other jobs, to prevent ES index to be stuck behind
bigger batch process.

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 38 Kyle M Hall 2022-04-11 14:15:45 UTC
Created attachment 133161 [details] [review]
Bug 27783: Add --job-type to background_jobs_worker.pl

This will let sysop adjust the number of workers and how they want to
manage them.
For instance one could want to have one worker for ES indexation and
another worker for other jobs, to prevent ES index to be stuck behind
bigger batch process.

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 39 Kyle M Hall 2022-04-11 14:16:15 UTC
Created attachment 133162 [details] [review]
Bug 27783: Replace --job-type by --queue

This patch adds a new column background_jobs.queue, which default to
'default'
By default, new jobs are enqueued into this default queue, and the
background job worker will subscribe to the default queue unless told
otherwise by the --job-queue option

The new job UpdateElasticIndex is automatically enqueued in another
queue named 'index'
So you can have 'default' worker with
    misc/background_jobs_worker.pl
and a dedicated indexing worker with
    misc/background_jobs_worker.pl --queue index

This is to address bug 27344 comment #15

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 40 Kyle M Hall 2022-04-11 14:16:19 UTC
Created attachment 133163 [details] [review]
Bug 27783: Add missing atomicupdate file

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 41 Kyle M Hall 2022-04-11 14:16:23 UTC
Created attachment 133164 [details] [review]
Bug 27783: (follow-up) Add DB change to installer

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 42 Kyle M Hall 2022-04-11 14:16:27 UTC
Created attachment 133165 [details] [review]
Bug 27783: Rename queues and adjust currently defined jobs

This patch renames the queues so the default is the **real-time** one, and
the other (*long_tasks*) is kept for **long running** tasks.

All current *batch* tasks are explicitly assigned to the **long_tasks**
queue as well.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 43 Kyle M Hall 2022-04-11 14:16:32 UTC
Created attachment 133166 [details] [review]
Bug 27783: (QA follow-up) Make dbrev idempotent

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 44 Kyle M Hall 2022-04-11 14:16:37 UTC
Created attachment 133167 [details] [review]
Bug 27783: DBIC update [DO NOT PUSH]

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 45 Kyle M Hall 2022-04-11 14:16:43 UTC
Created attachment 133168 [details] [review]
Bug 27783: Add queue handling in packaging tools

This patch adds queue name handling in is_worker_running and a handy
method to centralize process name handling for queue-specific workers.

To test:
1. Apply this patch
2. Run:
   $ source debian/scripts/koha-functions.sh
   $ get_worker_name kohadev
   kohadev-koha-worker
=> SUCCESS: Default queue means no queue name included
   $ get_worker_name kohadev default
   kohadev-koha-worker
=> SUCCESS: Default queue means no queue name included
   $ get_worker_name kohadev long_tasks
   kohadev-koha-worker-long_tasks
=> SUCCESS: Queue name appended to the name
4. Run:
   $ perl misc4dev/cp_debian_files.pl \
              --koha_dir /kohadevbox/koha \
              --gitify /kohadevbox/gitify \
              --instance kohadev
5. Run:
   $ koha-worker --start --queue oleonard kohadev
=> SUCCESS: Starts!
6. Check:
   $ ps waux | grep oleonard
=> SUCCESS: It is running
7. Play with --stop, --restart, --status and different queue names,
   including 'default' and without the --queue param as well.
=> SUCCESS: All as expected!
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 46 Kyle M Hall 2022-04-11 14:16:47 UTC
Created attachment 133169 [details] [review]
Bug 27783: (QA follow-up) Make dbrev executable

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 47 Martin Renvoize 2022-04-11 15:13:26 UTC
Created attachment 133170 [details] [review]
Bug 27783: Add --job-type to background_jobs_worker.pl

This will let sysop adjust the number of workers and how they want to
manage them.
For instance one could want to have one worker for ES indexation and
another worker for other jobs, to prevent ES index to be stuck behind
bigger batch process.

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 48 Martin Renvoize 2022-04-11 15:13:30 UTC
Created attachment 133171 [details] [review]
Bug 27783: Replace --job-type by --queue

This patch adds a new column background_jobs.queue, which default to
'default'
By default, new jobs are enqueued into this default queue, and the
background job worker will subscribe to the default queue unless told
otherwise by the --job-queue option

The new job UpdateElasticIndex is automatically enqueued in another
queue named 'index'
So you can have 'default' worker with
    misc/background_jobs_worker.pl
and a dedicated indexing worker with
    misc/background_jobs_worker.pl --queue index

This is to address bug 27344 comment #15

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 49 Martin Renvoize 2022-04-11 15:13:35 UTC
Created attachment 133172 [details] [review]
Bug 27783: Add missing atomicupdate file

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 50 Martin Renvoize 2022-04-11 15:13:39 UTC
Created attachment 133173 [details] [review]
Bug 27783: (follow-up) Add DB change to installer

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 51 Martin Renvoize 2022-04-11 15:13:44 UTC
Created attachment 133174 [details] [review]
Bug 27783: Rename queues and adjust currently defined jobs

This patch renames the queues so the default is the **real-time** one, and
the other (*long_tasks*) is kept for **long running** tasks.

All current *batch* tasks are explicitly assigned to the **long_tasks**
queue as well.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 52 Martin Renvoize 2022-04-11 15:13:48 UTC
Created attachment 133175 [details] [review]
Bug 27783: (QA follow-up) Make dbrev idempotent

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 53 Martin Renvoize 2022-04-11 15:13:53 UTC
Created attachment 133176 [details] [review]
Bug 27783: DBIC update [DO NOT PUSH]

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 54 Martin Renvoize 2022-04-11 15:13:57 UTC
Created attachment 133177 [details] [review]
Bug 27783: Add queue handling in packaging tools

This patch adds queue name handling in is_worker_running and a handy
method to centralize process name handling for queue-specific workers.

To test:
1. Apply this patch
2. Run:
   $ source debian/scripts/koha-functions.sh
   $ get_worker_name kohadev
   kohadev-koha-worker
=> SUCCESS: Default queue means no queue name included
   $ get_worker_name kohadev default
   kohadev-koha-worker
=> SUCCESS: Default queue means no queue name included
   $ get_worker_name kohadev long_tasks
   kohadev-koha-worker-long_tasks
=> SUCCESS: Queue name appended to the name
4. Run:
   $ perl misc4dev/cp_debian_files.pl \
              --koha_dir /kohadevbox/koha \
              --gitify /kohadevbox/gitify \
              --instance kohadev
5. Run:
   $ koha-worker --start --queue oleonard kohadev
=> SUCCESS: Starts!
6. Check:
   $ ps waux | grep oleonard
=> SUCCESS: It is running
7. Play with --stop, --restart, --status and different queue names,
   including 'default' and without the --queue param as well.
=> SUCCESS: All as expected!
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 55 Martin Renvoize 2022-04-11 15:14:02 UTC
Created attachment 133178 [details] [review]
Bug 27783: (QA follow-up) Make dbrev executable

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 56 Martin Renvoize 2022-04-11 15:14:07 UTC
Created attachment 133179 [details] [review]
Bug 27783: (follow-up) Rename options

Why require `job_`.. it's easiery to just use 'queue' and we have 'job'
from the context of the script we're calling.  This also inproves
consistency between the debian commands and the script params.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 57 Martin Renvoize 2022-04-11 15:17:24 UTC
Just another QA stamp here having re-tested the set.

This gives us two workers by default, one working through long-running tasks sequentially and the second working through short running tasks sequentially.. thus we can now work on short and long tasks in parallel and not cause hold-ups with a short running task waiting on a long-running one.

This gets us a good way towards where we want to be eventually with a configurable manager daemon with a config file and self spawning workers as Davids config file approach mentioned above suggests.. this is a great interim measure though for now and allows us to continue working on tasks whilst we consider the options for implementing such a manager daemon. (Job dependencies and things will need to be added too to allow for such a manager system to work).
Comment 58 Fridolin Somers 2022-04-13 13:56:57 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 59 Martin Renvoize 2022-04-29 14:13:16 UTC
*** Bug 30350 has been marked as a duplicate of this bug. ***
Comment 60 Jonathan Druart 2022-12-02 14:56:53 UTC
Why do we need the DB column background_jobs.queue exactly?
Comment 61 Jonathan Druart 2022-12-02 14:59:12 UTC
(In reply to Jonathan Druart from comment #60)
> Why do we need the DB column background_jobs.queue exactly?

... In case we are defaulting to the DB (no broker).