|
Description
Jonathan Druart
2021-01-05 14:04:45 UTC
Created attachment 114877 [details] [review] Bug 27344: Implement Elastic's update_index_background using Koha::BackgroundJob This patch adds a background job submodule, UpdateElasticIndex, to deal with async ES index update (not the deletion). Using NYTProf (on a checkin): Without 618ms, executing 35676 statements and 26355 subroutine calls in 266 source files and 83 string evals. With 521ms, executing 13282 statements and 7979 subroutine calls in 195 source files and 26 string evals. However there are some problems with this patch: 1. We don't want *all* the index update to be in the background_jobs table (we could add a filter on the list view) 2. We don't track the "progress" of the job as we are sending all the records to Elastic. It is okish in my opinion but it must be noted. Nick, can you have a look at this patch please? In brief testing this works, in addition to noted issues:
package Koha::BackgroundJob;
sub enqueue {
...
my $borrowernumber = C4::Context->userenv->{number}; # FIXME Handle non GUI calls
if I run a batch modification the indexing fails when called from the background job
Created attachment 116252 [details] [review] Bug 27344: Set job.borrowernumber to undef if enqueued from cli script I feel like this should be handled in it's own indexing daemon more like how we do zebra personally.. it feels a bit superfluous to requirements to add it into the general queue. (In reply to Martin Renvoize from comment #5) > I feel like this should be handled in it's own indexing daemon more like how > we do zebra personally.. it feels a bit superfluous to requirements to add > it into the general queue. Why another daemon when we have a generic mechanism to deal such background jobs? (In reply to Jonathan Druart from comment #6) > (In reply to Martin Renvoize from comment #5) > > I feel like this should be handled in it's own indexing daemon more like how > > we do zebra personally.. it feels a bit superfluous to requirements to add > > it into the general queue. > > Why another daemon when we have a generic mechanism to deal such background > jobs? Or at least a distinct worked for just this type of Job... I don't think it would be OK for these jobs to get queued behind other, perhaps slow running, background jobs. Thinking about this more.. I think we need to teach our worked daemon to accept a list of jobs it can work on.. that way we can utilise the background jobs processing whilst also allowing different workers to concentrate on different jobs. For this case, I think having a dedicated indexing worker would be the best option. I can work on that in another bug to not block this one. (In reply to Jonathan Druart from comment #1) > However there are some problems with this patch: > 1. We don't want *all* the index update to be in the background_jobs > table (we could add a filter on the list view) Why not.. do we not have a cleanup process to clean down completed jobs after a period? (In reply to Martin Renvoize from comment #8) > Thinking about this more.. I think we need to teach our worked daemon to > accept a list of jobs it can work on.. that way we can utilise the > background jobs processing whilst also allowing different workers to > concentrate on different jobs. For this case, I think having a dedicated > indexing worker would be the best option. > > I can work on that in another bug to not block this one. Yes, that's definitely something I had in mind when I wrote bug 22417, configuration for the different jobs. But I think by default it must be kept as simple as possible. Sysops who wants more sophisticate solutions will implement it (like if we need to modify the RabbitMQ config). Having an optional parameter to pass a list of "job type" is a good idea and could be implemented easily. (In reply to Martin Renvoize from comment #9) > (In reply to Jonathan Druart from comment #1) > > However there are some problems with this patch: > > 1. We don't want *all* the index update to be in the background_jobs > > table (we could add a filter on the list view) > > Why not.. do we not have a cleanup process to clean down completed jobs > after a period? Yes, that could be done. (In reply to Jonathan Druart from comment #1) > 2. We don't track the "progress" of the job as we are sending all the > records to Elastic. It is okish in my opinion but it must be noted. I don't see any way of getting at status information for bulk requests in elastic.. I don't think it's hugely required to be able to report this back to the end user anyway. (In reply to Martin Renvoize from comment #8) > For this case, I think having a dedicated > indexing worker would be the best option. 10/10 agreed Created attachment 116931 [details] [review] Bug 27344: Add --job-type to background_jobs_worker.pl This will let sysop adjust the number of workers and how they want to manage them. For instance one could want to have one worker for ES indexation and another worker for other jobs, to prevent ES index to be stuck behind bigger batch process. David, Martin, here it is! Hmm, I'm not contemplating whether we should actually be de-coupling 'jobs' from 'queues'.. or letting the 'type' option take a negation. Right now we just have the two jobs (three when this patch is added).. but if we see it getting real adoption we could end up with lots of different jobs.. it would become tedious listing all but one.. as such we might either want to say !update_elastic_index for one worker whilst update_elastic_index for another.. rather than having to list all remaining jobs on your non elastic worker. Other queuing systems appear to use 'queues' as a second distinction instead.. so we could have a 'default' queue that the non-elastic indexing tasks go onto and another for the elastic one.. that way we can differentiate into groups rather than having to list all types.. Thoughts? (In reply to Martin Renvoize from comment #15) > Other queuing systems appear to use 'queues' as a second distinction > instead.. so we could have a 'default' queue that the non-elastic indexing > tasks go onto and another for the elastic one.. that way we can > differentiate into groups rather than having to list all types.. Agreed. I think nominating queues rather than job types for workers makes more sense. I think --types is a good start.. however I think we aught to split this bug.. so we can discuss and work through queue improvements distinctly from this job.. Mind if I create a couple of bugs... one for --type (and I'll grab your patch and put it there if that's OK Jonathan).. and one for implementing queues. (In reply to Martin Renvoize from comment #17) > I think --types is a good start.. however I think we aught to split this > bug.. so we can discuss and work through queue improvements distinctly from > this job.. > > Mind if I create a couple of bugs... one for --type (and I'll grab your > patch and put it there if that's OK Jonathan).. and one for implementing > queues. I think that --type is likely to be unwieldy. Actually, I just realized that background_jobs_worker.pl has a design flaw regarding memory consumption, which will become more obvious when doing elastic indexing. I'll open a new task for that. (In reply to David Cook from comment #19) > Actually, I just realized that background_jobs_worker.pl has a design flaw > regarding memory consumption, which will become more obvious when doing > elastic indexing. I'll open a new task for that. Care to expand on this? (In reply to Martin Renvoize from comment #20) > (In reply to David Cook from comment #19) > > Actually, I just realized that background_jobs_worker.pl has a design flaw > > regarding memory consumption, which will become more obvious when doing > > elastic indexing. I'll open a new task for that. > > Care to expand on this? Ooops. I meant to link the new issue. Check out Bug 27756. Basically, Perl will never return memory back to the OS. It will re-use memory once it's freed internally within the Perl program, but any memory allocated to a Perl process will stay allocated by the OS for the life of that process. Just see Bug 27756 for more information heh. Do we still need this or we abandon? I still think it's a good idea myself. One to pursue next cycle :) Why isn't this moving forward? Patch doesn't apply on current master for me : <pre> arthur@h:~/repos/koha : $ git bz apply 27344 Bug 27344 - Implement Elastic's update_index_background using Koha::BackgroundJob 114877 - Bug 27344: Implement Elastic's update_index_background using Koha::BackgroundJob 116252 - Bug 27344: Set job.borrowernumber to undef if enqueued from cli script 116931 - Bug 27344: Add --job-type to background_jobs_worker.pl Apply? [(y)es, (n)o, (i)nteractive] y Application de Bug 27344: Implement Elastic's update_index_background using Koha::BackgroundJob Utilisation de l'information de l'index pour reconstruire un arbre de base... M Koha/BackgroundJob.pm M Koha/SearchEngine/Elasticsearch/Indexer.pm M koha-tmpl/intranet-tmpl/prog/en/modules/admin/background_jobs.tt M misc/background_jobs_worker.pl Retour à un patch de la base et fusion à 3 points... Fusion automatique de misc/background_jobs_worker.pl CONFLIT (contenu) : Conflit de fusion dans misc/background_jobs_worker.pl Fusion automatique de koha-tmpl/intranet-tmpl/prog/en/modules/admin/background_jobs.tt CONFLIT (contenu) : Conflit de fusion dans koha-tmpl/intranet-tmpl/prog/en/modules/admin/background_jobs.tt Fusion automatique de Koha/SearchEngine/Elasticsearch/Indexer.pm Fusion automatique de Koha/BackgroundJob.pm CONFLIT (contenu) : Conflit de fusion dans Koha/BackgroundJob.pm error: Échec d'intégration des modifications. l'application de la rustine a échoué à 0001 Bug 27344: Implement Elastic's update_index_background using Koha::BackgroundJob astuce: Utilisez 'git am --show-current-patch=diff' pour visualiser le patch en échec When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-27344-Implement-Elastics-updateindexbackground-HVa3WT.patch </pre> Created attachment 132444 [details] [review] Bug 27344: Implement Elastic's update_index_background using Koha::BackgroundJob This patch adds a background job submodule, UpdateElasticIndex, to deal with async ES index update (not the deletion). Using NYTProf (on a checkin): Without 618ms, executing 35676 statements and 26355 subroutine calls in 266 source files and 83 string evals. With 521ms, executing 13282 statements and 7979 subroutine calls in 195 source files and 26 string evals. However there are some problems with this patch: 1. We don't want *all* the index update to be in the background_jobs table (we could add a filter on the list view) 2. We don't track the "progress" of the job as we are sending all the records to Elastic. It is okish in my opinion but it must be noted. Created attachment 132445 [details] [review] Bug 27344: Set job.borrowernumber to undef if enqueued from cli script Created attachment 132446 [details] [review] Bug 27344: Add --job-type to background_jobs_worker.pl This will let sysop adjust the number of workers and how they want to manage them. For instance one could want to have one worker for ES indexation and another worker for other jobs, to prevent ES index to be stuck behind bigger batch process. Created attachment 132586 [details] [review] Bug 27344: Replace --job-type by --job-queue This patch adds a new column background_jobs.queue, which default to 'default' By default, new jobs are enqueued into this default queue, and the background job worker will subscribe to the default queue unless told otherwise by the --job-queue option The new job UpdateElasticIndex is automatically enqueued in another queue named 'index' So you can have 'default' worker with misc/background_jobs_worker.pl and a dedicated indexing worker with misc/background_jobs_worker.pl --job-queue index This is to address bug 27344 comment #15 Created attachment 132609 [details] [review] Bug 27344: Add missing include file Created attachment 132610 [details] [review] Bug 27344: Implement Elastic's update_index_background using Koha::BackgroundJob This patch adds a background job submodule, UpdateElasticIndex, to deal with async ES index update (not the deletion). Using NYTProf (on a checkin): Without 618ms, executing 35676 statements and 26355 subroutine calls in 266 source files and 83 string evals. With 521ms, executing 13282 statements and 7979 subroutine calls in 195 source files and 26 string evals. However there are some problems with this patch: 1. We don't want *all* the index update to be in the background_jobs tabtle (we could add a filter on the list view) 2. We don't track the "progress" of the job as we are sending all the records to Elastic. It is okish in my opinion but it must be noted. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 132611 [details] [review] Bug 27344: Set job.borrowernumber to undef if enqueued from cli script Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 132612 [details] [review] Bug 27344: Add --job-type to background_jobs_worker.pl This will let sysop adjust the number of workers and how they want to manage them. For instance one could want to have one worker for ES indexation and another worker for other jobs, to prevent ES index to be stuck behind bigger batch process. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 132613 [details] [review] Bug 27344: Replace --job-type by --job-queue This patch adds a new column background_jobs.queue, which default to 'default' By default, new jobs are enqueued into this default queue, and the background job worker will subscribe to the default queue unless told otherwise by the --job-queue option The new job UpdateElasticIndex is automatically enqueued in another queue named 'index' So you can have 'default' worker with misc/background_jobs_worker.pl and a dedicated indexing worker with misc/background_jobs_worker.pl --job-queue index This is to address bug 27344 comment #15 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Right.. I'm going to grab Julians final patch and move it to bug 27783 as I think it needs a little more thought/work. (Including adding a missing DB update that adds the new column and update existing jobs with their queue).. Then we can QA this piece in isolation. Created attachment 132645 [details] [review] Bug 27344: Add missing include file Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com> Created attachment 132646 [details] [review] Bug 27344: (QA follow-up) typo Created attachment 132794 [details] [review] Bug 27344: Implement Elastic's update_index_background using Koha::BackgroundJob This patch adds a background job submodule, UpdateElasticIndex, to deal with async ES index update (not the deletion). Using NYTProf (on a checkin): Without 618ms, executing 35676 statements and 26355 subroutine calls in 266 source files and 83 string evals. With 521ms, executing 13282 statements and 7979 subroutine calls in 195 source files and 26 string evals. However there are some problems with this patch: 1. We don't want *all* the index update to be in the background_jobs tabtle (we could add a filter on the list view) 2. We don't track the "progress" of the job as we are sending all the records to Elastic. It is okish in my opinion but it must be noted. Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 132795 [details] [review] Bug 27344: Set job.borrowernumber to undef if enqueued from cli script Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 132796 [details] [review] Bug 27344: Add --job-type to background_jobs_worker.pl This will let sysop adjust the number of workers and how they want to manage them. For instance one could want to have one worker for ES indexation and another worker for other jobs, to prevent ES index to be stuck behind bigger batch process. Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 132797 [details] [review] Bug 27344: Add missing include file Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 132798 [details] [review] Bug 27344: (QA follow-up) typo Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 132843 [details] [review] Bug 27344: Implement Elastic's update_index_background using Koha::BackgroundJob This patch adds a background job submodule, UpdateElasticIndex, to deal with async ES index update (not the deletion). Using NYTProf (on a checkin): Without 618ms, executing 35676 statements and 26355 subroutine calls in 266 source files and 83 string evals. With 521ms, executing 13282 statements and 7979 subroutine calls in 195 source files and 26 string evals. However there are some problems with this patch: 1. We don't want *all* the index update to be in the background_jobs tabtle (we could add a filter on the list view) 2. We don't track the "progress" of the job as we are sending all the records to Elastic. It is okish in my opinion but it must be noted. Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 132844 [details] [review] Bug 27344: Add missing include file Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 132845 [details] [review] Bug 27344: Adapt to bug 30181 changes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> This got broken at some point
data: {"messages":[{"code":"index_error","type":"error","error":"Can't locate object method \"new\" via package \"Koha::SearchEngine::Indexer\" (perhaps you forgot to load \"Koha::SearchEngine::Indexer\"?) at /kohadevbox/koha/Koha/BackgroundJob/UpdateElasticIndex.pm line 78.\n"}],"record_ids":["14"],"report":{"total_records":1,"total_success":0},"record_server":"biblioserver"}
Created attachment 132969 [details] [review] Bug 27344: Add missing imports Created attachment 132970 [details] [review] Bug 27344: Warn the error in the log Otherwise info about the failure is hidden in DB Minor: Progress is 0/1 Batch item modification (for instance) will run in background, but will also run a new task for the indexation. I don't think we should run another task for the indexation in case of batch operations. (In reply to Jonathan Druart from comment #50) > Batch item modification (for instance) will run in background, but will also > run a new task for the indexation. > I don't think we should run another task for the indexation in case of batch > operations. This is your patch, right? We just moved the job_type/queue stuff to another bug. I have opened several bug reports. There is a lot of work to provide before considering this one ready. I am not sure about the relations between the bug reports, but I think at least bug 30465 is a blocking this one. Otherwise we will see 1 task + X indexation tasks when you update X biblio records using the batch record modification tools. Created attachment 132994 [details] [review] Bug 27344: Fix tests Created attachment 132995 [details] [review] Bug 27344: Fix call to update_index with invalid ids Can't use an undefined value as an ARRAY reference at /kohadevbox/koha/Koha/SearchEngine/Elasticsearch/Indexer.pm line 121. Caught that when working on follow-up bug reports. Ready for testing again. I think we should push all the related bug reports along with this one. Created attachment 133033 [details] [review] Bug 27344: Add error detail if something wrong happened I've pushed a remote branch with the different dependencies applied https://gitlab.com/joubu/Koha/-/commits/bug_27344 If we decide to enqueue a new task for the indexing step (for the other background jobs), we just need to remove the { index_now => 1 } flag we pass to ->index_records. Created attachment 134406 [details] [review] Bug 27344: Implement Elastic's update_index_background using Koha::BackgroundJob This patch adds a background job submodule, UpdateElasticIndex, to deal with async ES index update (not the deletion). Using NYTProf (on a checkin): Without 618ms, executing 35676 statements and 26355 subroutine calls in 266 source files and 83 string evals. With 521ms, executing 13282 statements and 7979 subroutine calls in 195 source files and 26 string evals. However there are some problems with this patch: 1. We don't want *all* the index update to be in the background_jobs tabtle (we could add a filter on the list view) 2. We don't track the "progress" of the job as we are sending all the records to Elastic. It is okish in my opinion but it must be noted. Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 134407 [details] [review] Bug 27344: Add missing include file Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 134408 [details] [review] Bug 27344: Adapt to bug 30181 changes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 134409 [details] [review] Bug 27344: Add missing imports Created attachment 134410 [details] [review] Bug 27344: Warn the error in the log Otherwise info about the failure is hidden in DB Created attachment 134411 [details] [review] Bug 27344: Fix tests Created attachment 134412 [details] [review] Bug 27344: Fix call to update_index with invalid ids Can't use an undefined value as an ARRAY reference at /kohadevbox/koha/Koha/SearchEngine/Elasticsearch/Indexer.pm line 121. Caught that when working on follow-up bug reports. Created attachment 134413 [details] [review] Bug 27344: Add error detail if something wrong happened Pushed to master for 22.05, thanks to everybody involved [U+1F984] |