Description
Didier Gautheron
2022-11-21 11:56:47 UTC
Created attachment 144156 [details] [review] Bug 32305: Only process jobs in 'new' state I've created Bug 32330 to optimize query of background_jobs_worker.pl when pooling jobs directly from database I am not sure this is the correct place, shouldn't it be in Koha::BackgroundJob->process? Created attachment 144278 [details] [review] [ALTERNATIVE-PATCH] Bug 32305: Don't process a job if its status is not 'new' Patch for discussion, will need tests if we go with it. I think Jonathans approach is the right way to go.. Though I must admit, I found it confusing when looking at the 'start' helper method too.. that already checks status and throws an exception if the status isn't new. Having both 'process' and 'start' methods is a tad confusing. Created attachment 144389 [details] [review] Bug 32305: Don't process a job if its status is not 'new' Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Should we force a refetch from the DB just prior to checking this status.. just in case we have a stale object? (In reply to Martin Renvoize from comment #8) > Should we force a refetch from the DB just prior to checking this status.. > just in case we have a stale object? Sounds like a good idea to me (In reply to Martin Renvoize from comment #8) > Should we force a refetch from the DB just prior to checking this status.. > just in case we have a stale object? I suppose this goes back to what Jonathan was saying at bug 32395... Please tell me what we are still discussing here? I came from 30943 which is mnore or less linked to this dev. Let's move forward with Jonathan's approach, and please add the refetch. We then make bug 32395 depend on this one, for further cleanup. I don't understand why we would need a fetch here, and I actually think we should not get from db. The ->process call is from the worker, which should happen right after ->find. If the object does not have a status==new then we should explode as the developer certainly messed up with something. Looking here (In reply to Jonathan Druart from comment #5) > Patch for discussion, will need tests if we go with it. This speaks for itself :) How much sense does it make to check the status in the base object if most background jobs override it with their own? Hmm, looks like we always start in the base object and go to derived class.. And note that derived often begins with a ->start that contains an exception already for a status <> new. Should we add the start in the general process instead ? (In reply to Jonathan Druart from comment #13) > I don't understand why we would need a fetch here, and I actually think we > should not get from db. > > The ->process call is from the worker, which should happen right after > ->find. If the object does not have a status==new then we should explode as > the developer certainly messed up with something. Sounds good to me. I won't have more time to dedicate to this, feel free to take over. (In reply to Jonathan Druart from comment #18) > I won't have more time to dedicate to this, feel free to take over. I will now in light of the pending bug 34990. My solution is resolve the inconsistency between MQ and DB "mode": MQ my $job = Koha::BackgroundJobs->find($args->{job_id}); DB my $jobs = Koha::BackgroundJobs->search({ status => 'new', queue => \@queues }); We should do the same checks for MQ mode, so: my $job = Koha::BackgroundJobs->search( { id => $args->{job_id}, status => 'new, queue => \@queues } ); So check status new AND check queue. Created attachment 157256 [details] [review] Bug 32305: Worker should ignore wrong status or queue in MQ mode MQ mode: The worker has seen that MQ is running and consumes messages instead of polling db only. Test plan: Stop worker for long tasks. Make sure that Rabbit MQ runs. Stage a file. (This adds a long task.) Goto staff view of jobs and cancel this job. Check if job is still in MQ with rabbitmqctl list_queues. Now start worker for long tasks. Check if job is gone in MQ with rabbitmqctl list_queues. And check logfile for the adjusted warning like: [WARN] Job 5 not found, or has wrong status/queue main:: /usr/share/koha/misc/workers/background_jobs_worker.pl (134) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> We should do it too for es_indexer_daemon.pl Created attachment 157320 [details] [review] Bug 32305: Counterpart for es_indexer_daemon.pl This is a kind of copy-and-paste variant of background worker. We probably could do better than having two scripts here ;) Test plan: See former test plan. Apply it to ES indexing. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 157321 [details] [review] Bug 32305: Worker should ignore wrong status or queue in MQ mode MQ mode: The worker has seen that MQ is running and consumes messages instead of polling db only. Test plan: Stop worker for long tasks. Make sure that Rabbit MQ runs. Stage a file. (This adds a long task.) Goto staff view of jobs and cancel this job. Check if job is still in MQ with rabbitmqctl list_queues. Now start worker for long tasks. Check if job is gone in MQ with rabbitmqctl list_queues. And check logfile for the adjusted warning like: [WARN] Job 5 not found, or has wrong status/queue main:: /usr/share/koha/misc/workers/background_jobs_worker.pl (134) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 157322 [details] [review] Bug 32305: Counterpart for es_indexer_daemon.pl This is a kind of copy-and-paste variant of background worker. We probably could do better than having two scripts here ;) Test plan: See former test plan. Apply it to ES indexing. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Please QA. We need this one to go with 34990. See also omnibus. Note for QA: Jonathan suggested to move the selection to a filter_by method in BackgroundJobs. I agree. We should do that not on the compact bugfix here imo, but make that part of some refactoring as described further on the omnibus. QAing now. Created attachment 157496 [details] [review] Bug 32305: Worker should ignore wrong status or queue in MQ mode MQ mode: The worker has seen that MQ is running and consumes messages instead of polling db only. Test plan: Stop worker for long tasks. Make sure that Rabbit MQ runs. Stage a file. (This adds a long task.) Goto staff view of jobs and cancel this job. Check if job is still in MQ with rabbitmqctl list_queues. Now start worker for long tasks. Check if job is gone in MQ with rabbitmqctl list_queues. And check logfile for the adjusted warning like: [WARN] Job 5 not found, or has wrong status/queue main:: /usr/share/koha/misc/workers/background_jobs_worker.pl (134) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 157497 [details] [review] Bug 32305: Counterpart for es_indexer_daemon.pl This is a kind of copy-and-paste variant of background worker. We probably could do better than having two scripts here ;) Test plan: See former test plan. Apply it to ES indexing. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> All looking good here.. yes, it would be great to move to a filter_by.. but I totally agree it's OK to do that as part of the wider refactoring work going on. Passing QA Created attachment 157500 [details] [review] Bug 32305: Worker should ignore wrong status or queue in MQ mode MQ mode: The worker has seen that MQ is running and consumes messages instead of polling db only. Test plan: Stop worker for long tasks. Make sure that Rabbit MQ runs. Stage a file. (This adds a long task.) Goto staff view of jobs and cancel this job. Check if job is still in MQ with rabbitmqctl list_queues. Now start worker for long tasks. Check if job is gone in MQ with rabbitmqctl list_queues. And check logfile for the adjusted warning like: [WARN] Job 5 not found, or has wrong status/queue main:: /usr/share/koha/misc/workers/background_jobs_worker.pl (134) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Removed queue from query at MQ side. Discussed on IRC. Created attachment 157501 [details] [review] Bug 32305: Counterpart for es_indexer_daemon.pl This is a kind of copy-and-paste variant of background worker. We probably could do better than having two scripts here ;) Test plan: See former test plan. Apply it to ES indexing. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Removed queue from query at MQ side. Discussed on IRC. From IRC: [11:00] <Joubu> if the worker is getting an ID from the MQ it's not possible that the ID is from another queue. That should be true indeed. Being cautious.. I think we can remove the queue check.. it's superflous and could lead to confusion later once we've all forgotten this discussion. Happy for that to be a quick follow-up please :) (In reply to Martin Renvoize from comment #35) > I think we can remove the queue check.. it's superflous and could lead to > confusion later once we've all forgotten this discussion. > > Happy for that to be a quick follow-up please :) See above. Done Pushed to master for 23.11. Nice work everyone, thanks! Pushed to 23.05.x for 23.05.05 Nice work everyone! Pushed to oldstable for 22.11.x |