[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 #65 from Jonathan Druart <jonathan.druart at bugs.koha-community.org> ---
(In reply to David Cook from comment #63)
> (In reply to Jonathan Druart from comment #60)
> > Yes sure, I agree. I will implement that as soon as this is PQA.
> 
> The latest work still seems like work-in-progress to me? 

Yes and no. It's ready for testing. I will address QA comment once this will
hit the QA process.
We will certainly deal with other additions on separate bug reports.

> - Connection details and credentials are still hard-coded into
> Koha::BackgroundJob, and there is no way of passing in a virtual host (which
> we might not use out of the box, but it would be good to build in the
> support)
> - For instance, koha_worker.pl should be moved to
> "./misc/bin/koha_worker.pl".

It's in misc/background_jobs_worker.pl
the koha_worker.pl script is part of a "DO NOT PUSH" patch and is there to
understand what's going on.
Please read the test plan from comment 30.

> - I was thinking the "namespace" for the queues should use the database name
> instead, since non-Debian installs may or may not have correctly set
> memcached_namespace. 

Yes, it's in the comment:
 # This namespace is wrong, it must be a vhost instead.
 # But to do so it needs to be created on the server => much more work when a
new Koha instance is created.
 # Also, here we just want the Koha instance's name, but it's not in the
config...
 # Picking a random id (memcached_namespace) from the config

That will be addressed on bug 25674.

> - I don't think there's any service to start up koha_worker.pl?

There is a patch titled "Add debian script koha-worker".

> - I'm curious why koha_worker.pl doesn't use the /queue prefix (as suggested
> by https://www.rabbitmq.com/stomp.html). I think it works without it, as I
> have tested your patches, but I'm curious.

It's also in Net::Stomp's POD, so I guess it's a good pattern to follow. I will
submit a follow-up.

> - Could we use a "/" instead of a "-" for the queue destination. I think
> that would be more conventional.

Agreed.

> - Koha::BackgroundJob loads C4::MarcModificationTemplates and C4::Biblio but
> doesn't use them.

Yes, leftover. Thanks!

> I'd really like to see this code be plugin friendly from Day 1, which I
> think could be done with some updates to koha_worker.pl and
> Koha::BackgroundJob. At the moment, koha_worker.pl and Koha::BackgroundJob
> are hard-coded with 2 job_types. It wouldn't be difficult to move the
> job_types somewhere more configurable.

It won't from Day 1. I really need a sign from the community that we agree on a
first move.
Make it more complex now is not a good idea.
Once we agreed on that I promise to move all the different background jobs we
have in a short term. That will show us some pattern that is repeated and must
be refactored.

For instance when I rebased yesterday I had to fix a conflict with bug 18127
(see patch "Restore the 'add to list' feature"). It highlights an interesting
feature that must be implemented: a post-process hook to display something
specific to the job's report. I hard-coded it for now, but that would be
something great to implement in the next steps, especially for the plugins.

(In reply to David Cook from comment #64)
> (In reply to David Cook from comment #62)
> > (In reply to Jonathan Druart from comment #60)
> > > Yes sure, I agree. I will implement that as soon as this is PQA.
> > 
> > Could you squash your commits and then post a patch here? 
> > 
> 
> If you squash your commits and post a patch here, I'd be happy to
> collaborate on this work.

I would prefer to keep the history. Checking out a remote branch should not
prevent you to collaborate, it's even more easier than attaching the patches
from here :)

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