Bug 27344 - Implement Elastic's update_index_background using Koha::BackgroundJob
Summary: Implement Elastic's update_index_background using Koha::BackgroundJob
Status: In Discussion
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: Testopia
URL:
Keywords:
Depends on:
Blocks: 27341
  Show dependency treegraph
 
Reported: 2021-01-05 14:04 UTC by Jonathan Druart
Modified: 2021-11-25 22:44 UTC (History)
7 users (show)

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


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

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 :)