https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22417
--- Comment #158 from Jonathan Druart <jonathan.dru...@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. _______________________________________________ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/