ModBiblio must implement skip_index and we should update the search engine's index after all the biblio records have been modified.
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.
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.
(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?
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.
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.
(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.
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.
Created attachment 134404 [details] [review] Bug 30465: (QA follow-up) Enqueue indexing Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 134405 [details] [review] Bug 30465: (QA follow-up) POD fixes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(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.
(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.
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.
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.
(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.
(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.
(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.
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>
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.
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
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 ?
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.
On second look, I don't understand the switch from index_records to update_index here.. was there a reason for that Jonathan?
Created attachment 134676 [details] [review] Bug 30465: Fix Koha/Items/BatchUpdate.t
Created attachment 134711 [details] [review] Bug 30465: Fix Koha/Items/BatchUpdate.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Great, I agree this is the way to go to.. added my SO to Jonathans patch :)
Follow-up pushed as RM Assistant to push us closer to a green Jenkins.