Bug 35920 - Centralize code from workers
Summary: Centralize code from workers
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 33898 35819
Blocks: 35092
  Show dependency treegraph
 
Reported: 2024-01-26 15:12 UTC by Jonathan Druart
Modified: 2025-04-30 04:16 UTC (History)
6 users (show)

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


Attachments
Bug 35920: Centralize code from workers (13.86 KB, patch)
2024-01-26 15:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35920: Centralize worker code (12.29 KB, patch)
2024-03-08 11:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35920: Use BackgroundWorker in es_indexer_daemon (7.94 KB, patch)
2024-03-08 11:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[Poll-only proposal] Bug 35920: Move worker code to class (13.91 KB, patch)
2025-04-29 18:33 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2024-01-26 15:12:49 UTC
With bug 35819 we will have too much code duplicated in worker scripts.

It needs to be moved to a module.
Comment 1 Jonathan Druart 2024-01-26 15:14:26 UTC
This is WIP. We need bug 35819 pushed first, then to clean this, it is not very nice. But at least it will prevent the code to be duplicated.
Comment 2 Jonathan Druart 2024-01-26 15:17:26 UTC
Created attachment 161544 [details] [review]
Bug 35920: Centralize code from workers
Comment 3 Tomás Cohen Arazi (tcohen) 2024-01-31 17:47:10 UTC
I like the concept.

Given the discussion Marcel is driving around rabbit vs. polling, why not create some base class and then different classes implementing each communication mechanism? That way each use case would be self-contained and maintainble (i.e. one can fix a bug without possibly breaking the other use case, etc).

Something like:

Koha::Worker
Koha::Worker::STOMP
Koha::Worker::Polling

and then the `background_jobs_worker.pl` script would just call


if ( C4::Context->use_stomp ) {
    Koha::Worker->new( 'STOMP' )->run();
}
else {
    Koha::Worker->new( 'Polling' )->run();
}
Comment 4 Marcel de Rooy 2024-02-01 07:12:37 UTC
(In reply to Tomás Cohen Arazi from comment #3)
> I like the concept.
> 
> Given the discussion Marcel is driving around rabbit vs. polling, why not
> create some base class and then different classes implementing each
> communication mechanism? That way each use case would be self-contained and
> maintainble (i.e. one can fix a bug without possibly breaking the other use
> case, etc).
> 
> Something like:
> 
> Koha::Worker
> Koha::Worker::STOMP
> Koha::Worker::Polling
> 
> and then the `background_jobs_worker.pl` script would just call
> 
> 
> if ( C4::Context->use_stomp ) {
>     Koha::Worker->new( 'STOMP' )->run();
> }
> else {
>     Koha::Worker->new( 'Polling' )->run();
> }

Looks promising! Lets go there. Less_code_in_scripts++
Comment 5 Jonathan Druart 2024-02-01 07:25:07 UTC
(In reply to Tomás Cohen Arazi from comment #3)
> I like the concept.
> 
> Given the discussion Marcel is driving around rabbit vs. polling, why not
> create some base class and then different classes implementing each
> communication mechanism? 

The discussion is going into a "drop one of the other" conclusion, so it does not seem useful to provide a temporary abstraction that will be useless later...
Comment 6 Tomás Cohen Arazi (tcohen) 2024-02-05 15:33:42 UTC
(In reply to Jonathan Druart from comment #5)
> (In reply to Tomás Cohen Arazi from comment #3)
> > I like the concept.
> > 
> > Given the discussion Marcel is driving around rabbit vs. polling, why not
> > create some base class and then different classes implementing each
> > communication mechanism? 
> 
> The discussion is going into a "drop one of the other" conclusion, so it
> does not seem useful to provide a temporary abstraction that will be useless
> later...

But... will we make a decision anytime soon? This is a blocker IMHO, for a maintainable implementation.
Comment 7 Marcel de Rooy 2024-02-06 12:49:58 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> (In reply to Jonathan Druart from comment #5)
> > (In reply to Tomás Cohen Arazi from comment #3)
> > > I like the concept.
> > > 
> > > Given the discussion Marcel is driving around rabbit vs. polling, why not
> > > create some base class and then different classes implementing each
> > > communication mechanism? 
> > 
> > The discussion is going into a "drop one of the other" conclusion, so it
> > does not seem useful to provide a temporary abstraction that will be useless
> > later...
> 
> But... will we make a decision anytime soon? This is a blocker IMHO, for a
> maintainable implementation.

Lack of response and available time are the usual suspects :)
I did not happen to see your own response on bug 35092 btw..
Comment 8 Marcel de Rooy 2024-03-08 10:58:18 UTC
Giving this a try on top of 33898
Comment 9 Marcel de Rooy 2024-03-08 11:03:28 UTC
Created attachment 162951 [details] [review]
Bug 35920: Centralize worker code

Test plan:
Confirm that running jobs still works as expected.
Restart workers and try to stage a MARC file and import it.
Comment 10 Marcel de Rooy 2024-03-08 11:03:30 UTC
Created attachment 162952 [details] [review]
Bug 35920: Use BackgroundWorker in es_indexer_daemon

This needs incorporating the batch_size in the main loop.

Test plan:
Test both regular worker and es indexer daemon here.
Comment 11 Marcel de Rooy 2024-03-08 11:06:19 UTC
(In reply to Jonathan Druart from comment #1)
> This is WIP. We need bug 35819 pushed first, then to clean this, it is not
> very nice. But at least it will prevent the code to be duplicated.

Hmm. Didnt notice your patch when I did something similar with patch 2 and 3..
Comment 12 Jonathan Druart 2024-03-08 12:45:40 UTC
(In reply to Marcel de Rooy from comment #11)
> (In reply to Jonathan Druart from comment #1)
> > This is WIP. We need bug 35819 pushed first, then to clean this, it is not
> > very nice. But at least it will prevent the code to be duplicated.
> 
> Hmm. Didnt notice your patch when I did something similar with patch 2 and
> 3..

Too bad to duplicate the effort again.

And so what?
The goal of my approach was to split Rabbit and DBMS. And also move broker's responsibilities out of Koha::BackgroundJob
Comment 13 Marcel de Rooy 2024-03-22 07:32:39 UTC
Comment on attachment 162951 [details] [review]
Bug 35920: Centralize worker code

Abandoning it. Asked for feedback, received none, etc.
Comment 14 Jonathan Druart 2024-03-25 13:23:09 UTC
(In reply to Marcel de Rooy from comment #13)
> Comment on attachment 162951 [details] [review] [review]
> Bug 35920: Centralize worker code
> 
> Abandoning it. Asked for feedback, received none, etc.

Seen the previous comment? What about my patch then?
Comment 15 Marcel de Rooy 2024-03-25 13:39:43 UTC
(In reply to Jonathan Druart from comment #14)
> (In reply to Marcel de Rooy from comment #13)
> > Comment on attachment 162951 [details] [review] [review] [review]
> > Bug 35920: Centralize worker code
> > 
> > Abandoning it. Asked for feedback, received none, etc.
> 
> Seen the previous comment? What about my patch then?

It is still here. Status is not changed.
Comment 16 Jonathan Druart 2024-03-25 13:57:07 UTC
No opinion on it?
Comment 17 Marcel de Rooy 2024-03-25 14:01:12 UTC
(In reply to Jonathan Druart from comment #16)
> No opinion on it?

I happened to ignore the whole patch when I wrote the others. But viewing diagonally, I would like to go that direction. So yes. But you must have had your reasons to keep it in NEW at the time.
Comment 18 Martin Renvoize (ashimema) 2024-04-16 12:06:07 UTC
I like the direction of travel here too.. but the NEW status doesn't really highlight if it's ready for testing.. feedback.. or what?
Comment 19 Jonathan Druart 2024-04-16 12:15:49 UTC
(In reply to Martin Renvoize from comment #18)
> I like the direction of travel here too.. but the NEW status doesn't really
> highlight if it's ready for testing.. feedback.. or what?

It's for discussion. Happy to continue if we agree on it.
Tomas suggested something else but didn't provide an alternative patch.
Comment 20 Tomás Cohen Arazi (tcohen) 2024-10-15 12:36:22 UTC
I like Jonathan's code, but we really need to decide if we want to keep Rabbit or not. I don't think we have discussed/voted it at all. I understand RabbitMQ has some advocates. I'm not sure I have the time right now to mock some code. Will do my best.
Comment 21 Jonathan Druart 2024-10-15 12:59:10 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #20)
> I like Jonathan's code, but we really need to decide if we want to keep
> Rabbit or not. I don't think we have discussed/voted it at all. I understand
> RabbitMQ has some advocates. I'm not sure I have the time right now to mock
> some code. Will do my best.

Mock, what for? provide tests?

I am pretty sure we discussed this at the hackfest and decided to keep both. We can re-discussed every 6 months or move forward :D
Comment 22 David Cook 2025-03-13 23:21:47 UTC
I haven't read any of the comments yet, but +1 to deduplicating code
Comment 23 Tomás Cohen Arazi (tcohen) 2025-04-29 18:33:08 UTC
Created attachment 181684 [details] [review]
[Poll-only proposal] Bug 35920: Move worker code to class

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi (tcohen) 2025-04-29 18:36:41 UTC
Ok, my two cents.

Please open your mind and take a look at how the code looks when we factor out STOMP, frames and its messaging edge cases.

This code is not 100% functional because the current background job classes are still obfuscated with frame-decoding compatibility (i.e. ->process expects to be passed the decoded contents from its own .data attribute) but this further adds to the argument about getting rid of Rabbit.
Comment 25 Kyle M Hall (khall) 2025-04-29 18:56:32 UTC
I have to agree with Tomas. My personal experience is that Rabbit has caused more issues than it has solved. It has been error prone, is complicated and complicates Koha's code more than necessary. I think it's more than what Koha needs and going back to a just a polling daemon would improve Koha's reliability greatly for background job handling.
Comment 26 David Cook 2025-04-30 02:24:35 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #24)
> Please open your mind and take a look at how the code looks when we factor
> out STOMP, frames and its messaging edge cases.
> 
> This code is not 100% functional because the current background job classes
> are still obfuscated with frame-decoding compatibility (i.e. ->process
> expects to be passed the decoded contents from its own .data attribute) but
> this further adds to the argument about getting rid of Rabbit.

(In reply to Kyle M Hall (khall) from comment #25)
> I have to agree with Tomas. My personal experience is that Rabbit has caused
> more issues than it has solved. It has been error prone, is complicated and
> complicates Koha's code more than necessary. I think it's more than what
> Koha needs and going back to a just a polling daemon would improve Koha's
> reliability greatly for background job handling.

My personal experience has been that RabbitMQ has worked really well - both in Koha and in multiple other projects. There have been a couple of issues over time, but nothing that wasn't easy to fix. RabbitMQ gives us possibilities for a future where a polling daemon won't be enough. Many systems these days implement message brokers like RabbitMQ or ActiveMQ without issue. To me, the biggest obstacle has been people not willing to update their skills and knowledge, which admittedly does make it more difficult to move forward with RabbitMQ. It feels like we are being asked to settle for the lowest common denominator rather than the best tool for the job. 

That said, regardless of what I personally think about it, it seems that we have 2 different groups which are unlikely to change their minds.

How about a config in koha-conf.xml or /etc/default/koha-common that lets you decide between DB polling or RabbitMQ? 

my $worker = Koha::Worker->new({ broker => $broker });
$worker->run( { max_processes => $max_processes, queues => \@queues } );

Jonathan and I could do Koha::Worker::RabbitMQ and other folks could do Koha::Worker::DB. 

They both need to be able to use the same incoming job JSON and store the same result JSON. Other than that... it shouldn't matter.
Comment 27 David Cook 2025-04-30 02:29:55 UTC
Alternatively, let's put it to a vote? 

I think that RabbitMQ is a great tool for Koha, but if the majority disagree then it doesn't make sense to keep going with it. I would be disappointed by that decision, but that is the nature of groupwork; not everyone gets to be happy. 

If we did choose to proceed just with the DB broker, I think it would still be wise to adopt the syntax I suggested to make it easier to add support for other brokers again in the future if we wanted to re-adopt RabbitMQ (or some other broker), if it proved necessary. 

Anyway, this isn't a hill that I plan on dying on. Just sharing my 2 cents in the hope that it helps move things along.
Comment 28 Tomás Cohen Arazi (tcohen) 2025-04-30 02:45:33 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #3)
> I like the concept.
> 
> Given the discussion Marcel is driving around rabbit vs. polling, why not
> create some base class and then different classes implementing each
> communication mechanism? That way each use case would be self-contained and
> maintainble (i.e. one can fix a bug without possibly breaking the other use
> case, etc).
> 
> Something like:
> 
> Koha::Worker
> Koha::Worker::STOMP
> Koha::Worker::Polling
> 
> and then the `background_jobs_worker.pl` script would just call
> 
> 
> if ( C4::Context->use_stomp ) {
>     Koha::Worker->new( 'STOMP' )->run();
> }
> else {
>     Koha::Worker->new( 'Polling' )->run();
> }

This was my original idea, pretty much what David suggests.

I was trying to write it this morning when I hit a roadblock: I wanted to prepare it to support delayed retries, and figured it would require manually installing and maintaining the setup for the delayed queue plugin (not shipped in Debian).

And I understood this would need to be set to not-default for the general public because of that. So a niche use case with too little gain so far. So the question is: who’s gonna maintain that? Implementing a feature would require doing it in both for sure. Who would do it?

Those are my questions.
Comment 29 David Cook 2025-04-30 03:03:04 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #28)
> This was my original idea, pretty much what David suggests.

Sorry for missing that before!
 
> I was trying to write it this morning when I hit a roadblock: I wanted to
> prepare it to support delayed retries, and figured it would require manually
> installing and maintaining the setup for the delayed queue plugin (not
> shipped in Debian).
>
> And I understood this would need to be set to not-default for the general
> public because of that. So a niche use case with too little gain so far. So
> the question is: who’s gonna maintain that? Implementing a feature would
> require doing it in both for sure. Who would do it?
> 
> Those are my questions.

Yeah, I just saw that bug report. I've added some comments there. I think that the solution for delayed retries should be found in a task scheduler rather than a job worker. That's even the solution suggested by the developer of that RabbitMQ plugin.

That's actually something I bumped into with bug 10662 because I was performing OAI-PMH harvests using background job workers, and I needed to be able to schedule future harvests. I did that by having the worker add a task to the task scheduler.
Comment 30 David Cook 2025-04-30 03:07:48 UTC
(In reply to David Cook from comment #29)
> That's actually something I bumped into with bug 10662 because I was
> performing OAI-PMH harvests using background job workers, and I needed to be
> able to schedule future harvests. I did that by having the worker add a task
> to the task scheduler.

I wrote that task scheduler, which was high performance and had 1 second granularity. We wouldn't have to do something as sophisticated as that. 

Many years ago, I think that Galen suggested a cronjob-driven task scheduler with a database table for storing the tasks. The downside is that the minimum granularity for scheduling is 1 minute, but that should cover most current cases.

We could do a daemon (like I did years ago), but that does add complexity. I think my task scheduler scared people, and it was event-driven which did make it difficult to test (although since then I've thought of ways I could've unit tested the functionality regardless of the event-driven engine).
Comment 31 Tomás Cohen Arazi (tcohen) 2025-04-30 03:08:57 UTC
(In reply to David Cook from comment #29)
> (In reply to Tomás Cohen Arazi (tcohen) from comment #28)
> > This was my original idea, pretty much what David suggests.
> 
> Sorry for missing that before!
>  
> > I was trying to write it this morning when I hit a roadblock: I wanted to
> > prepare it to support delayed retries, and figured it would require manually
> > installing and maintaining the setup for the delayed queue plugin (not
> > shipped in Debian).
> >
> > And I understood this would need to be set to not-default for the general
> > public because of that. So a niche use case with too little gain so far. So
> > the question is: who’s gonna maintain that? Implementing a feature would
> > require doing it in both for sure. Who would do it?
> > 
> > Those are my questions.
> 
> Yeah, I just saw that bug report. I've added some comments there. I think
> that the solution for delayed retries should be found in a task scheduler
> rather than a job worker. That's even the solution suggested by the
> developer of that RabbitMQ plugin.
> 
> That's actually something I bumped into with bug 10662 because I was
> performing OAI-PMH harvests using background job workers, and I needed to be
> able to schedule future harvests. I did that by having the worker add a task
> to the task scheduler.

Compare adding a column `run_after` of type timestamp with adding yet another subsystem to maintain. We need to match timezones, I need to sleep now, this is a great chat
Comment 32 David Cook 2025-04-30 03:27:32 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #31)
> Compare adding a column `run_after` of type timestamp with adding yet
> another subsystem to maintain. We need to match timezones, I need to sleep
> now, this is a great chat

I knew you were going to say that ;).

But I think that you're still conflating the task scheduler, message passing, and task/job workers. They're all separate functions.

We could add a "run_at" column to background_jobs. We then have a task scheduler poll the database for any jobs where run_at <= now(). 

Once we've got all the jobs scheduled to be run, we then need to pass the message. If we use the DB style, we can just launch our child worker processes directly so the message is passed in memory (or send them via a Unix socket to a worker process). If we use the RabbitMQ style, we can send the messages to the broker, and the worker processes consume the messages. 

Currently, we don't have a task scheduler. With RabbitMQ, we just pass the message immediately and the job worker consumes it immediately. With the DB model, we wait for the daemon to pick up the job from the DB and pass the message through its own memory.

--

Do you see that regardless of whether or not you have RabbitMQ, adding "run_at" would essentially create a new subsystem?
Comment 33 David Cook 2025-04-30 03:36:47 UTC
Reflecting on what I've just written there... adding this task scheduler functionality could actually alleviate some of the issues that people have with the RabbitMQ. 

I'd have to double-check, but with the task scheduler I wrote in the past, I think that I used to store the PID of the worker against a task (I used a JSON file for saving internal state instead of the DB but same-same). So if processes restarted, the task scheduler could check if an "in progress" task's PID is still there. If it is, it's assumed it's still working on it. If it's not, then it can safely restart the task.

Anyway... happy to help with POCs on this. It doesn't have to be complicated.
Comment 34 David Cook 2025-04-30 04:07:02 UTC
I suppose the only question is whether all background jobs have to be scheduled tasks. 

If so, then that does eliminate some of the speed/performance advantages of RabbitMQ, because it would be captive to the task scheduler that polls the database based on a slow interval.

Of course, the task scheduler does not have to poll the database - the task scheduler I wrote for bug 10662 was reachable by Unix socket from the Koha backend. It could easily handle tasks scheduled for ASAP/now() or the future.

If we did that, we get back to the message passing. For RabbitMQ, you'd just send the message to the broker. Easy. For the DB, do we communicate with a background_jobs.pl daemon? Do we set something special in the DB, so that background_jobs.pl can poll for it? Currently, it sounds like you'd want to fork the worker processes from there? Because if everything is forked from the 1 parent, it's easier to manage?

I understand the appeal of putting all the code into 1 monolithic daemon, but it's also the most custom non-standard way of doing it. 

And I suppose here it would actually be 2 monolithic daemons? Because background_jobs_worker.pl and es_indexer_daemon.pl would both be their own schedulers, message passers, workers. 

--

In an ideal world, we'd have a task scheduler daemon, RabbitMQ as the message broker, and then the worker daemons. Koha would schedule tasks, the task scheduler would pass the message at the appropriate time, and then the workers would consume the message. 

Everything would just be doing its own function using only the system resources that they need. And you could scale up/scale down as necessary depending on the size of your installation.

This is a pattern that other systems use, and I think it's easy to understand.
Comment 35 David Cook 2025-04-30 04:16:15 UTC
Anyway I've written too much once again.

Background processing is a passion of mine. The lack of a task scheduler is an oversight I've always regretted with this background processing work.