Bug 27344 - Implement Elastic's update_index_background using Koha::BackgroundJob
Summary: Implement Elastic's update_index_background using Koha::BackgroundJob
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords: release-notes-needed
Depends on: 27783
Blocks: 30462 30463 30360 30459 30460 30464 30465 30467
  Show dependency treegraph
 
Reported: 2021-01-05 14:04 UTC by Jonathan Druart
Modified: 2022-05-13 16:03 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Text to go in the release notes:
Version(s) released in:
22.05.00


Attachments
Bug 27344: Implement Elastic's update_index_background using Koha::BackgroundJob (12.22 KB, patch)
2021-01-05 14:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27344: Set job.borrowernumber to undef if enqueued from cli script (931 bytes, patch)
2021-02-03 12:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27344: Add --job-type to background_jobs_worker.pl (2.67 KB, patch)
2021-02-17 09:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27344: Implement Elastic's update_index_background using Koha::BackgroundJob (11.54 KB, patch)
2022-03-29 14:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27344: Set job.borrowernumber to undef if enqueued from cli script (933 bytes, patch)
2022-03-29 14:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27344: Add --job-type to background_jobs_worker.pl (2.75 KB, patch)
2022-03-29 14:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27344: Replace --job-type by --job-queue (6.26 KB, patch)
2022-03-30 12:28 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 27344: Add missing include file (1.52 KB, patch)
2022-03-30 13:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27344: Implement Elastic's update_index_background using Koha::BackgroundJob (11.59 KB, patch)
2022-03-30 13:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27344: Set job.borrowernumber to undef if enqueued from cli script (996 bytes, patch)
2022-03-30 13:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27344: Add --job-type to background_jobs_worker.pl (2.81 KB, patch)
2022-03-30 13:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27344: Replace --job-type by --job-queue (6.43 KB, patch)
2022-03-30 13:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27344: Add missing include file (1.57 KB, patch)
2022-03-30 15:13 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 27344: (QA follow-up) typo (705 bytes, patch)
2022-03-30 15:19 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 27344: Implement Elastic's update_index_background using Koha::BackgroundJob (11.65 KB, patch)
2022-03-31 15:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27344: Set job.borrowernumber to undef if enqueued from cli script (1.03 KB, patch)
2022-03-31 15:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27344: Add --job-type to background_jobs_worker.pl (2.87 KB, patch)
2022-03-31 15:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27344: Add missing include file (1.63 KB, patch)
2022-03-31 15:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27344: (QA follow-up) typo (828 bytes, patch)
2022-03-31 15:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27344: Implement Elastic's update_index_background using Koha::BackgroundJob (11.24 KB, patch)
2022-04-01 12:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27344: Add missing include file (1.63 KB, patch)
2022-04-01 12:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27344: Adapt to bug 30181 changes (2.13 KB, patch)
2022-04-01 12:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27344: Add missing imports (765 bytes, patch)
2022-04-05 12:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27344: Warn the error in the log (815 bytes, patch)
2022-04-05 12:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27344: Fix tests (2.68 KB, patch)
2022-04-06 12:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27344: Fix call to update_index with invalid ids (2.20 KB, patch)
2022-04-06 12:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27344: Add error detail if something wrong happened (1.41 KB, patch)
2022-04-06 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27344: Implement Elastic's update_index_background using Koha::BackgroundJob (11.29 KB, patch)
2022-04-29 17:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27344: Add missing include file (1.63 KB, patch)
2022-04-29 17:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27344: Adapt to bug 30181 changes (2.13 KB, patch)
2022-04-29 17:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27344: Add missing imports (763 bytes, patch)
2022-04-29 17:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27344: Warn the error in the log (813 bytes, patch)
2022-04-29 17:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27344: Fix tests (2.68 KB, patch)
2022-04-29 17:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27344: Fix call to update_index with invalid ids (2.19 KB, patch)
2022-04-29 17:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27344: Add error detail if something wrong happened (1.41 KB, patch)
2022-04-29 17:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-01-05 14:04:45 UTC

    
Comment 1 Jonathan Druart 2021-01-05 14:28:16 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.
Comment 2 Jonathan Druart 2021-01-13 15:49:46 UTC
Nick, can you have a look at this patch please?
Comment 3 Nick Clemens 2021-01-14 15:36:23 UTC
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
Comment 4 Jonathan Druart 2021-02-03 12:41:19 UTC
Created attachment 116252 [details] [review]
Bug 27344: Set job.borrowernumber to undef if enqueued from cli script
Comment 5 Martin Renvoize 2021-02-08 15:27:24 UTC
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.
Comment 6 Jonathan Druart 2021-02-08 16:11:36 UTC
(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?
Comment 7 Martin Renvoize 2021-02-08 17:04:29 UTC
(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.
Comment 8 Martin Renvoize 2021-02-09 12:33:24 UTC
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.
Comment 9 Martin Renvoize 2021-02-09 12:52:36 UTC
(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?
Comment 10 Jonathan Druart 2021-02-09 13:03:19 UTC
(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.
Comment 11 Martin Renvoize 2021-02-09 13:21:12 UTC
(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.
Comment 12 David Cook 2021-02-09 22:24:03 UTC
(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
Comment 13 Jonathan Druart 2021-02-17 09:54:21 UTC
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.
Comment 14 Jonathan Druart 2021-02-17 09:54:48 UTC
David, Martin, here it is!
Comment 15 Martin Renvoize 2021-02-19 14:35:16 UTC
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?
Comment 16 David Cook 2021-02-22 03:09:02 UTC
(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.
Comment 17 Martin Renvoize 2021-02-22 08:32:48 UTC
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.
Comment 18 David Cook 2021-02-22 23:02:58 UTC
(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.
Comment 19 David Cook 2021-02-22 23:03:49 UTC
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.
Comment 20 Martin Renvoize 2021-02-24 19:43:38 UTC
(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?
Comment 21 David Cook 2021-02-24 23:20:30 UTC
(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.
Comment 22 Jonathan Druart 2021-11-25 10:44:50 UTC
Do we still need this or we abandon?
Comment 23 Martin Renvoize 2021-11-25 22:44:48 UTC
I still think it's a good idea myself. One to pursue next cycle :)
Comment 24 Jonathan Druart 2022-02-16 06:51:10 UTC
Why isn't this moving forward?
Comment 25 Arthur Suzuki 2022-03-29 11:20:17 UTC
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>
Comment 26 Jonathan Druart 2022-03-29 14:30:02 UTC
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.
Comment 27 Jonathan Druart 2022-03-29 14:30:07 UTC
Created attachment 132445 [details] [review]
Bug 27344: Set job.borrowernumber to undef if enqueued from cli script
Comment 28 Jonathan Druart 2022-03-29 14:30:12 UTC
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.
Comment 29 Julian Maurice 2022-03-30 12:28:03 UTC
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
Comment 30 Jonathan Druart 2022-03-30 13:20:45 UTC
Created attachment 132609 [details] [review]
Bug 27344: Add missing include file
Comment 31 Martin Renvoize 2022-03-30 13:36:55 UTC
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>
Comment 32 Martin Renvoize 2022-03-30 13:37:01 UTC
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>
Comment 33 Martin Renvoize 2022-03-30 13:37:06 UTC
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>
Comment 34 Martin Renvoize 2022-03-30 13:37:11 UTC
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>
Comment 35 Martin Renvoize 2022-03-30 14:11:20 UTC
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.
Comment 36 Arthur Suzuki 2022-03-30 15:13:05 UTC
Created attachment 132645 [details] [review]
Bug 27344: Add missing include file

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 37 Arthur Suzuki 2022-03-30 15:19:04 UTC
Created attachment 132646 [details] [review]
Bug 27344: (QA follow-up) typo
Comment 38 Martin Renvoize 2022-03-31 15:44:18 UTC
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>
Comment 39 Martin Renvoize 2022-03-31 15:44:23 UTC
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>
Comment 40 Martin Renvoize 2022-03-31 15:44:27 UTC
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>
Comment 41 Martin Renvoize 2022-03-31 15:44:32 UTC
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>
Comment 42 Martin Renvoize 2022-03-31 15:44:38 UTC
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>
Comment 43 Tomás Cohen Arazi 2022-04-01 12:00:46 UTC
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>
Comment 44 Tomás Cohen Arazi 2022-04-01 12:00:54 UTC
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>
Comment 45 Tomás Cohen Arazi 2022-04-01 12:00:59 UTC
Created attachment 132845 [details] [review]
Bug 27344: Adapt to bug 30181 changes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 46 Jonathan Druart 2022-04-05 12:48:09 UTC
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"}
Comment 47 Jonathan Druart 2022-04-05 12:51:28 UTC
Created attachment 132969 [details] [review]
Bug 27344: Add missing imports
Comment 48 Jonathan Druart 2022-04-05 12:53:03 UTC
Created attachment 132970 [details] [review]
Bug 27344: Warn the error in the log

Otherwise info about the failure is hidden in DB
Comment 49 Jonathan Druart 2022-04-05 13:00:04 UTC
Minor: Progress is 0/1
Comment 50 Jonathan Druart 2022-04-05 13:01:25 UTC
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.
Comment 51 Tomás Cohen Arazi 2022-04-06 08:44:55 UTC
(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.
Comment 52 Jonathan Druart 2022-04-06 09:56:18 UTC
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.
Comment 53 Jonathan Druart 2022-04-06 12:33:24 UTC
Created attachment 132994 [details] [review]
Bug 27344: Fix tests
Comment 54 Jonathan Druart 2022-04-06 12:39:19 UTC
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.
Comment 55 Jonathan Druart 2022-04-06 12:40:27 UTC
Ready for testing again. I think we should push all the related bug reports along with this one.
Comment 56 Jonathan Druart 2022-04-06 14:11:47 UTC
Created attachment 133033 [details] [review]
Bug 27344: Add error detail if something wrong happened
Comment 57 Jonathan Druart 2022-04-08 07:23:49 UTC
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.
Comment 58 Tomás Cohen Arazi 2022-04-29 17:23:12 UTC
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>
Comment 59 Tomás Cohen Arazi 2022-04-29 17:23:22 UTC
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>
Comment 60 Tomás Cohen Arazi 2022-04-29 17:23:31 UTC
Created attachment 134408 [details] [review]
Bug 27344: Adapt to bug 30181 changes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 61 Tomás Cohen Arazi 2022-04-29 17:23:41 UTC
Created attachment 134409 [details] [review]
Bug 27344: Add missing imports
Comment 62 Tomás Cohen Arazi 2022-04-29 17:23:50 UTC
Created attachment 134410 [details] [review]
Bug 27344: Warn the error in the log

Otherwise info about the failure is hidden in DB
Comment 63 Tomás Cohen Arazi 2022-04-29 17:23:59 UTC
Created attachment 134411 [details] [review]
Bug 27344: Fix tests
Comment 64 Tomás Cohen Arazi 2022-04-29 17:24:07 UTC
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.
Comment 65 Tomás Cohen Arazi 2022-04-29 17:24:15 UTC
Created attachment 134413 [details] [review]
Bug 27344: Add error detail if something wrong happened
Comment 66 Fridolin Somers 2022-05-03 21:27:28 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄