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 (vote)
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: 2024-04-16 12:15 UTC (History)
5 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:


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

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 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 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 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.