With bug 35819 we will have too much code duplicated in worker scripts. It needs to be moved to a module.
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.
Created attachment 161544 [details] [review] Bug 35920: Centralize code from workers
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(); }
(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++
(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...
(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.
(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..
Giving this a try on top of 33898
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.
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.
(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..
(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 on attachment 162951 [details] [review] Bug 35920: Centralize worker code Abandoning it. Asked for feedback, received none, etc.
(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?
(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.
No opinion on it?
(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.
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?
(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.
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.
(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