https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22417 --- Comment #158 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Thanks Marcel and Kyle for having a look at this! (In reply to Marcel de Rooy from comment #156)
QA Comment: Great to see us moving to a message queue in Koha ! Nice start. And surely things to discuss ;) The UI changes are not really the most relevant imo. We could start as an experimental feature too if needed (in that case we should not touch batch mod). But we should add imo some configuration options (see below, setting up vhost, user, etc.) And perhaps even more important and already commented on a bit, we are adding a task queue here with a message queue. Obviously, we could send other stuff into the MQ; so this might become a problem later on. And sounding a bit nasty, but not meant to be: We are saving the data into a background job table as well as sending it to MQ. Aren't we being redundant here? Actually, it looks like our background job could just hook to the background jobs table and process what is left there. So what is the real benefit here in this implementation of the MQ exactly?
I am not sure I understand what you are suggesting. Do you mean misc/background_jobs_worker.pl could watch the DB table and so we could remove the RabbitMQ dependency?
Detailed comments: my $stomp = Net::Stomp->new( { hostname => 'localhost', port => '61613' } ); hardcoded port number / we should probably also let rabbitmq run in its own container, so replace localhost too: move to koha-conf No disconnect ? Memory issues when not doing so? $stomp->connect( { login => 'guest', passcode => 'guest' } ); hmm; isnt this the place for vhosts to come in ? just as vhost, a user should have been created? / hardcoded: move to koha-conf or separate conf?
If we need to tweak the configuration it could go into koha-conf. As I said in a comment (Koha::BackgroundJob->enqueue), specific vhosts need to be defined server-side. Here we just want to have it working out of the box. We will have to focus specifically on that depending on the needs we found once it's pushed.
# Picking a random id (memcached_namespace) from the config my $namespace = C4::Context->config('memcached_namespace'); Shouldnt we configure a koha vhost during install (koha-create) and indeed pick that from koha-conf or so ? Why not use config/database instead? Normally it looks like koha_instance
As Kyle said, it does not really matter for now. We just need an ID.
Not sure btw if using the term namespace is confusing here; or just a queue prefix ?
Maybe you are right but it does not matter much :)
Add Koha::BackgroundJob::BatchUpdateBiblio Not sure if we need that. The crux is here only four lines: my $record = C4::Biblio::GetMarcBiblio({ biblionumber => $biblionumber }); C4::MarcModificationTemplates::ModifyRecordWithTemplate( $mmtid, $record ); my $frameworkcode = C4::Biblio::GetFrameworkCode( $biblionumber ); C4::Biblio::ModBiblio( $record, $biblionumber, $frameworkcode ); The rest of the code is more or less overhead which perhaps better could be moved to the general module. The module should care for iterating too. So if we could pass a sub or method to a general process method along with a params hash here including biblionumber and marc_template_id, it might be enough? Similar remarks for enqueue. Would reduce a number of submodules. So we should enqueue: Koha::Backgroundjob->new({ type => 'batch_record_mod', ... })->enqueue({ mmtid => $mmtid, records => $records }) And process with Koha::Backgroundjobs->process({ type => ... }) where the process method iterates over the records and associates type with e.g. Koha::Biblios::batch_update [four lines above] etc.
Yes, maybe. The idea was to be flexible in case we need it. When we will have more job moved to this mechanism we will know what can be refactored. Also I think it's good to have more code in the module than in the controller scripts.
installer/data/mysql/atomicupdate/bug_15032.perl Wrong bug number
Indeed, it's actually from 15032. It has a long history :)
diff koha_worker.pl misc/background_jobs_worker.pl Why did you add koha_worker.pl? We already run the misc script ?
new_koha_job.pl and koha_worker.pl won't be pushed. They are there for testing purpose only.
No tests anywhere [yet]? Ok :)
For point. My (very) bad tentative to justify would be that existing code was not covered by tests either. Argument rejected. I could try to write some tests for the 2 ->process method, but I don't think ->enqueue or ->connect can be tested easily. -- You are receiving this mail because: You are watching all bug changes.