[Bug 30465] New: BatchUpdateBiblio task does not deal with indexation correctly
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Bug ID: 30465 Summary: BatchUpdateBiblio task does not deal with indexation correctly Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: jonathan.druart+koha@gmail.com QA Contact: testopia@bugs.koha-community.org Blocks: 27344 ModBiblio must implement skip_index and we should update the search engine's index after all the biblio records have been modified. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27344 [Bug 27344] Implement Elastic's update_index_background using Koha::BackgroundJob -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |27344 Blocks|27344 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27344 [Bug 27344] Implement Elastic's update_index_background using Koha::BackgroundJob -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|koha-bugs@lists.koha-commun |jonathan.druart+koha@gmail. |ity.org |com -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 --- Comment #1 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 132988 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=132988&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au, | |julian.maurice@biblibre.com | |, | |martin.renvoize@ptfs-europe | |.com, tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 --- Comment #2 from David Cook <dcook@prosentient.com.au> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 --- Comment #3 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #132988|0 |1 is obsolete| | --- Comment #4 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 133057 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=133057&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #133057|0 |1 is obsolete| | --- Comment #5 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 133058 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=133058&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 --- Comment #6 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #133058|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 --- Comment #7 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 134403 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=134403&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 --- Comment #8 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 134404 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=134404&action=edit Bug 30465: (QA follow-up) Enqueue indexing Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 --- Comment #9 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 134405 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=134405&action=edit Bug 30465: (QA follow-up) POD fixes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 --- Comment #10 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Tomás Cohen Arazi from comment #8)
Created attachment 134404 [details] [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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 --- Comment #11 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #10)
(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.
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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 --- Comment #12 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #134403|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #134404|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #134405|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 --- Comment #13 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 134465 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=134465&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |martin.renvoize@ptfs-europe |y.org |.com --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 --- Comment #15 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 --- Comment #16 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_22_05_candidate -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #134465|0 |1 is obsolete| | --- Comment #17 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 134645 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=134645&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #18 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |27341 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27341 [Bug 27341] Performance 21.05 - Omnibus -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |22.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 --- Comment #19 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to master for 22.05, thanks to everybody involved 🦄 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m Keywords| |additional_work_needed --- Comment #20 from Fridolin Somers <fridolin.somers@biblibre.com> --- 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 ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 --- Comment #21 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 --- Comment #22 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- On second look, I don't understand the switch from index_records to update_index here.. was there a reason for that Jonathan? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 --- Comment #23 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 134676 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=134676&action=edit Bug 30465: Fix Koha/Items/BatchUpdate.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #134676|0 |1 is obsolete| | --- Comment #24 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 134711 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=134711&action=edit Bug 30465: Fix Koha/Items/BatchUpdate.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 --- Comment #25 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Great, I agree this is the way to go to.. added my SO to Jonathans patch :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 --- Comment #26 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Follow-up pushed as RM Assistant to push us closer to a green Jenkins. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed, | |rel_22_05_candidate | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Bug 30465 depends on bug 27344, which changed state. Bug 27344 Summary: Implement Elastic's update_index_background using Koha::BackgroundJob https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27344 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30465 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com Resolution|--- |FIXED Status|Pushed to master |RESOLVED -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org