[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 #157 from Kyle M Hall <kyle at bywatersolutions.com> ---
(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 believe this is to prevent two workers from grabbing the same job. There may
be other solutions, such as row locking. I'm sure others are more well versed
in this subject than I am.

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

I think a separate conf makes the most sense, as workers are per-server and not
per-instance, right?

> # 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
> Not sure btw if using the term namespace is confusing here; or just a queue
> prefix ?

I see nothing wrong with what's being done here. The memcached_namespace is
just another way of saying 'instance name' really.


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

Unless they are expected to diverge more over time. I don't think this is a
blocker, but could be a followup bug.

> installer/data/mysql/atomicupdate/bug_15032.perl
> Wrong bug number
> 
> diff koha_worker.pl misc/background_jobs_worker.pl
> Why did you add koha_worker.pl? We already run the misc script ?
> 
> No tests anywhere [yet]? Ok :)

Also, many minor QA script failures that need to be cleaned up.

-- 
You are receiving this mail because:
You are watching all bug changes.