[Koha-bugs] [Bug 22417] Add a task queue

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22417

--- Comment #158 from Jonathan Druart <jonathan.druart at 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.