https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22417 --- Comment #65 from Jonathan Druart <jonathan.druart@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.