Bug 30465

Summary: BatchUpdateBiblio task does not deal with indexation correctly
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: dcook, fridolin.somers, julian.maurice, kyle, martin.renvoize, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00
Bug Depends on: 27344    
Bug Blocks: 27341    
Attachments: Bug 30465: Make BatchUpdateBiblio update the index in one request
Bug 30465: Make BatchUpdateBiblio update the index in one request
Bug 30465: Make BatchUpdateBiblio update the index in one request
Bug 30465: Make BatchUpdateBiblio update the index in one request
Bug 30465: (QA follow-up) Enqueue indexing
Bug 30465: (QA follow-up) POD fixes
Bug 30465: Make BatchUpdateBiblio update the index in one request
Bug 30465: Make BatchUpdateBiblio update the index in one request
Bug 30465: Fix Koha/Items/BatchUpdate.t
Bug 30465: Fix Koha/Items/BatchUpdate.t

Description Jonathan Druart 2022-04-06 09:53:54 UTC
ModBiblio must implement skip_index and we should update the search engine's index after all the biblio records have been modified.
Comment 1 Jonathan Druart 2022-04-06 11:54:38 UTC
Created attachment 132988 [details] [review]
Bug 30465: Make BatchUpdateBiblio update the index in one request

When using the batch record modification tool to modify several
bibliographic records, we don't want to send one index request per
biblio, we want to index them all on the fly after the records have been
modified.
Otherwise we will end up with one task per record, and records will be
indexed in background.

Test plan:
Use the batch mod tool to modify bibliographic records and confirm the
behaviour is correct.
Comment 2 David Cook 2022-04-06 23:53:38 UTC
I'm not sure I'll be much help, as it looks like this is an Elasticsearch specific dev? From the Zebra side I think we'd actually get worse performance with this change.
Comment 3 Jonathan Druart 2022-04-07 07:17:05 UTC
(In reply to David Cook from comment #2)
> I'm not sure I'll be much help, as it looks like this is an Elasticsearch
> specific dev? From the Zebra side I think we'd actually get worse
> performance with this change.

Why, can you detail please?
Comment 4 Jonathan Druart 2022-04-07 07:18:24 UTC
Created attachment 133057 [details] [review]
Bug 30465: Make BatchUpdateBiblio update the index in one request

When using the batch record modification tool to modify several
bibliographic records, we don't want to send one index request per
biblio, we want to index them all on the fly after the records have been
modified.
Otherwise we will end up with one task per record, and records will be
indexed in background.

Test plan:
Use the batch mod tool to modify bibliographic records and confirm the
behaviour is correct.
Comment 5 Jonathan Druart 2022-04-07 07:20:10 UTC
Created attachment 133058 [details] [review]
Bug 30465: Make BatchUpdateBiblio update the index in one request

When using the batch record modification tool to modify several
bibliographic records, we don't want to send one index request per
biblio, we want to index them all on the fly after the records have been
modified.
Otherwise we will end up with one task per record, and records will be
indexed in background.

Test plan:
Use the batch mod tool to modify bibliographic records and confirm the
behaviour is correct.
Comment 6 David Cook 2022-04-07 10:54:07 UTC
(In reply to Jonathan Druart from comment #3)
> (In reply to David Cook from comment #2)
> > I'm not sure I'll be much help, as it looks like this is an Elasticsearch
> > specific dev? From the Zebra side I think we'd actually get worse
> > performance with this change.
> 
> Why, can you detail please?

If I understand correctly, this change would mean that the zebraqueue would only be added to after all the updates are processed, right? At the moment, the zebraqueue is added to with each change, which means the koha-indexer can start processing them ASAP. In theory, this change could make it slower to get records indexed into Zebra.

However, if we're passing record IDs through RabbitMQ to a worker to index records in Elasticsearch, I think it could make sense to defer the indexing, so that the background worker can fetch X biblios from the database with 1 DB call before sending them to Elasticsearch. 

Anyway, I don't think it really matters, so feel free to ignore me hehe.
Comment 7 Tomás Cohen Arazi 2022-04-29 17:19:54 UTC
Created attachment 134403 [details] [review]
Bug 30465: Make BatchUpdateBiblio update the index in one request

When using the batch record modification tool to modify several
bibliographic records, we don't want to send one index request per
biblio, we want to index them all on the fly after the records have been
modified.
Otherwise we will end up with one task per record, and records will be
indexed in background.

Test plan:
Use the batch mod tool to modify bibliographic records and confirm the
behaviour is correct.
Comment 8 Tomás Cohen Arazi 2022-04-29 17:19:59 UTC
Created attachment 134404 [details] [review]
Bug 30465: (QA follow-up) Enqueue indexing

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2022-04-29 17:20:06 UTC
Created attachment 134405 [details] [review]
Bug 30465: (QA follow-up) POD fixes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Jonathan Druart 2022-05-02 07:50:54 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> Created attachment 134404 [details] [review] [review]
> Bug 30465: (QA follow-up) Enqueue indexing
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Can you explain why we need this patch?

Moreover the first patch is introducing the index_now param and you are removing the occurrence that is using it.
Comment 11 Tomás Cohen Arazi 2022-05-02 10:52:12 UTC
(In reply to Jonathan Druart from comment #10)
> (In reply to Tomás Cohen Arazi from comment #8)
> > Created attachment 134404 [details] [review] [review] [review]
> > Bug 30465: (QA follow-up) Enqueue indexing
> > 
> > Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
> 
> Can you explain why we need this patch?
> 
> Moreover the first patch is introducing the index_now param and you are
> removing the occurrence that is using it.

The patch on this bug does three main things:
1. Adds a 'skip_record_index' flag to ModBiblio/ModBiblioMarc so indexing is not triggered within those.
2. Makes the background job trigger indexing on the whole bibios set after the action has taken place.
3. Indexing is triggered within the background job, as part of it.

As I mentioned already in the hackfest, I'm against (3). I'm pretty sure indexing should always stand as its own enqueued task just in case ES/Zebra is not available or something wrong happens, and also because its failure shouldn't mean the batch task had failed, for example.

So most of the patch is correct (adding a way to signal not indexing, optimizing by indexing all at once) but I'd enqueue indexing. Hope it clarifies.
Comment 12 Jonathan Druart 2022-05-02 12:10:55 UTC
I disagree but I don't want to argue. I think it will be confusing for the end user to have 2 new jobs when only one has been requested.

If you want this patch as it then we need to remove the useless flag it introduces.
Comment 13 Tomás Cohen Arazi 2022-05-02 12:28:56 UTC
Created attachment 134465 [details] [review]
Bug 30465: Make BatchUpdateBiblio update the index in one request

When using the batch record modification tool to modify several
bibliographic records, we don't want to send one index request per
biblio, we want to index them all on the fly after the records have been
modified.
Otherwise we will end up with one task per record, and records will be
indexed in background.

Test plan:
Use the batch mod tool to modify bibliographic records and confirm the
behaviour is correct.
Comment 14 Tomás Cohen Arazi 2022-05-03 22:54:12 UTC
(In reply to Jonathan Druart from comment #12)
> I disagree but I don't want to argue. I think it will be confusing for the
> end user to have 2 new jobs when only one has been requested.

I believe indexing shouldn't be an end-user generated task, but a system one. Like real-time holds queue updating, it is not the end user requesting the holds queue update, they just checked-in an item. So maybe, in the end, it is a matter of how the information is presented to the end user.
Comment 15 David Cook 2022-05-03 23:25:53 UTC
(In reply to Tomás Cohen Arazi from comment #14)
> I believe indexing shouldn't be an end-user generated task, but a system
> one. Like real-time holds queue updating, it is not the end user requesting
> the holds queue update, they just checked-in an item. So maybe, in the end,
> it is a matter of how the information is presented to the end user.

I haven't been following the conversation super closely, but this reminds me of some thoughts I've had previously. 

When a user clicks "Add Foo", they should get feedback about the progress and completion of "Adding Foo". Behind the scenes, "Adding Foo" might involve its own asynchronous background tasks, but the user doesn't need to know that. They just need to know information about their user-initiated task.
Comment 16 Jonathan Druart 2022-05-04 06:53:13 UTC
(In reply to Tomás Cohen Arazi from comment #14)
> (In reply to Jonathan Druart from comment #12)
> > I disagree but I don't want to argue. I think it will be confusing for the
> > end user to have 2 new jobs when only one has been requested.
> 
> I believe indexing shouldn't be an end-user generated task, but a system
> one. Like real-time holds queue updating, it is not the end user requesting
> the holds queue update, they just checked-in an item. So maybe, in the end,
> it is a matter of how the information is presented to the end user.

A batch mod operation will be displayed as "done" on the detail view of the task, but actually an indexing process is still going on.
What you are suggesting is to have a link between the jobs (if I understand correctly), but it's not implement at all. I am not saying it's not the right way to do it, I am saying we are deciding to add confusion for not much benefit.
Comment 17 Martin Renvoize 2022-05-05 14:45:59 UTC
Created attachment 134645 [details] [review]
Bug 30465: Make BatchUpdateBiblio update the index in one request

When using the batch record modification tool to modify several
bibliographic records, we don't want to send one index request per
biblio, we want to index them all on the fly after the records have been
modified.
Otherwise we will end up with one task per record, and records will be
indexed in background.

Test plan:
Use the batch mod tool to modify bibliographic records and confirm the
behaviour is correct.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2022-05-05 14:48:00 UTC
OK.. whilst I agree this is detrimental to Zebra I'm not sure there's a simple way around it at this time..

We're using the bulk API for all indexing calls in Elastic it seems.. to me it feels like using the update API for some actions would be preferred, but that's more work.

I'd love to see actual on the fly indexing happening, i.e after each single record update we update the index.. but whilst we're always calling bulk api's it makes sense to group them.
Comment 19 Fridolin Somers 2022-05-05 21:21:14 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 20 Fridolin Somers 2022-05-06 02:19:14 UTC
This breaks unit test :

kohadev-koha@kohadevbox:/kohadevbox/koha$ prove t/db_dependent/Koha/Items/BatchUpdate.t 
t/db_dependent/Koha/Items/BatchUpdate.t ..     # No tests run!
t/db_dependent/Koha/Items/BatchUpdate.t .. 1/7 
#   Failed test 'No tests run for subtest "MARC subfield linked with kohafield"'
#   at t/db_dependent/Koha/Items/BatchUpdate.t line 78.
Can't locate object method "update_index" via package "Koha::SearchEngine::Zebra::Indexer" at /kohadevbox/koha/Koha/Items.pm line 360.
# Looks like your test exited with 11 just after 1.
t/db_dependent/Koha/Items/BatchUpdate.t .. Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 7/7 subtests

Indeed there is no update_index() in Koha::SearchEngine::Zebra::Indexer.
We should keep the old code in Koha/Items.pm line 360 right ?
Comment 21 Martin Renvoize 2022-05-06 06:09:58 UTC
Damn, I was so focused on ES of this one after QAing the others that I missed this.

I think I'd actually add the update_index method to the indexer instead to bring the zebra indexer a little closer to the es module. I believe it's just a matter off adding to zebraqueue.

I can take a closer look this morning.
Comment 22 Martin Renvoize 2022-05-06 06:41:02 UTC
On second look, I don't understand the switch from index_records to update_index here.. was there a reason for that Jonathan?
Comment 23 Jonathan Druart 2022-05-06 07:37:49 UTC
Created attachment 134676 [details] [review]
Bug 30465: Fix Koha/Items/BatchUpdate.t
Comment 24 Martin Renvoize 2022-05-06 08:49:47 UTC
Created attachment 134711 [details] [review]
Bug 30465: Fix Koha/Items/BatchUpdate.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2022-05-06 08:50:41 UTC
Great, I agree this is the way to go to.. added my SO to Jonathans patch :)
Comment 26 Martin Renvoize 2022-05-06 09:42:42 UTC
Follow-up pushed as RM Assistant to push us closer to a green Jenkins.