[Bug 30822] New: BatchCommit does not deal with indexation correctly
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 Bug ID: 30822 Summary: BatchCommit 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: martin.renvoize@ptfs-europe.com QA Contact: testopia@bugs.koha-community.org CC: dcook@prosentient.com.au, fridolin.somers@biblibre.com, jonathan.druart+koha@gmail.com, julian.maurice@biblibre.com, kyle@bywatersolutions.com, martin.renvoize@ptfs-europe.com, tomascohen@gmail.com Depends on: 27344 Blocks: 27341 AddBiblio must implement skip_index and we should update the search engine's index after all the biblio records have been imported. This also applies for bulkmarcimport. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27341 [Bug 27341] Performance 21.05 - Omnibus 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=30822 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=29440 -- 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=30822 --- Comment #1 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Is there a reason indexing could not take place during the import and would have to be pushed after the task is completed? Just wondering if this would not slow down things for big imports like you would do with bulkmarcimport. -- 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=30822 --- Comment #2 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (In reply to Katrin Fischer from comment #1)
Is there a reason indexing could not take place during the import and would have to be pushed after the task is completed? Just wondering if this would not slow down things for big imports like you would do with bulkmarcimport.
See discussion on bug 30465 for background on this exact issue... As far as I can tell there's already a bug in the existing implementation here anyway.. as I just mentioned in bug 29440 if `BiblioAddsAuthorities` is enabled we're using the search indexes to find authority matches as part of the AddBiblio action.. if we're calling a whole bunch of AddBiblio's in a row in a loop then the index will likely be out of date for at least some of these searches and as such authority matches won't work as expected.. we have a race condition. -- 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=30822 --- Comment #3 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- What we really need is task dependencies... then the Authority linking can be a task that's dependent on the rebuild having been completed. -- 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=30822 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |29440 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29440 [Bug 29440] Refactor/clean up bulkmarcimport.pl -- 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=30822 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |martin.renvoize@ptfs-europe |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=30822 --- Comment #4 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 135251 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135251&action=edit Bug 30822: Make BatchCommitRecords update the index in one request When committing staged marc imports to the catalogue we will often be importing a batch of records. We don't want to send one index request per biblio affected, we want to index them all after the records have been modified otherwise we will end up with multiple tasks per record (when items are also affected). Test plan: 1) Use the stage marc record tool to stage and commit a set of records and confirm the behaviour remains correct. 2) If using Elastic, check that only one indexing job is queued to take place resulting from the committed import. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=30824 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 Joonas Kylmälä <joonas.kylmala@iki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joonas.kylmala@iki.fi --- Comment #5 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- Good improvement! Just a small nit: as BatchCommitItems is a public function it should take either skip_record_index as a parameter or document clearly that it doesn't do indexing and it is left to the caller. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 --- Comment #6 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (In reply to Joonas Kylmälä from comment #5)
Good improvement! Just a small nit: as BatchCommitItems is a public function it should take either skip_record_index as a parameter or document clearly that it doesn't do indexing and it is left to the caller.
Good Catch.. I'm wondering if the third option is to rename the function to be 'private'.. it appears to only be called inside this module and isn't exported either. Certainly some POD around it would also be sensible though. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 135372 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135372&action=edit Bug 30822: Clarify that BatchCommitItems is a private function BatchCommitItems is only being used within this module and isn't mentioned in EXPORT_OK. This patch simply renames it to _batchCommitItems to take the _ standard for private functions and also adds a little hint to the POD of the function to clarify that the caller must trigger a re-index. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 Joonas Kylmälä <joonas.kylmala@iki.fi> 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=30822 Joonas Kylmälä <joonas.kylmala@iki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #135251|0 |1 is obsolete| | Attachment #135372|0 |1 is obsolete| | --- Comment #8 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- Created attachment 135963 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135963&action=edit Bug 30822: Make BatchCommitRecords update the index in one request When committing staged marc imports to the catalogue we will often be importing a batch of records. We don't want to send one index request per biblio affected, we want to index them all after the records have been modified otherwise we will end up with multiple tasks per record (when items are also affected). Test plan: 1) Use the stage marc record tool to stage and commit a set of records and confirm the behaviour remains correct. 2) If using Elastic, check that only one indexing job is queued to take place resulting from the committed import. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 --- Comment #9 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- Created attachment 135964 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135964&action=edit Bug 30822: Clarify that BatchCommitItems is a private function BatchCommitItems is only being used within this module and isn't mentioned in EXPORT_OK. This patch simply renames it to _batchCommitItems to take the _ standard for private functions and also adds a little hint to the POD of the function to clarify that the caller must trigger a re-index. JK: Amended patch to rename also the function in t/db_dependent/ImportBatch.t and fix typo "commiting" => "commiting" Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |27421 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27421 [Bug 27421] Porting tools/stage-marc-import.pl to BackgroundJob -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 --- Comment #10 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- I think you did better than me on https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=136129 But maybe we need that part, for biblio deletions, what do you think? + if ( $record_type eq 'biblio' && ( @updated_biblionumbers || @deleted_biblionumbers ) ) { + my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX }); + if ( @deleted_biblionumbers ) { + $indexer->index_records( \@deleted_biblionumbers, "recordDelete", "biblioserver" ); + } else { + $indexer->index_records( \@updated_biblionumbers, "specialUpdate", "biblioserver" ); + } + } -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply --- Comment #11 from David Cook <dcook@prosentient.com.au> --- Went to apply it as a dependency of bug 27421 and noticed patches don't apply anymore: Bug 30822 - BatchCommit does not deal with indexation correctly 135963 - Bug 30822: Make BatchCommitRecords update the index in one request 135964 - Bug 30822: Clarify that BatchCommitItems is a private function Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 30822: Make BatchCommitRecords update the index in one request Using index info to reconstruct a base tree... M C4/ImportBatch.pm Falling back to patching base and 3-way merge... Auto-merging C4/ImportBatch.pm CONFLICT (content): Merge conflict in C4/ImportBatch.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 30822: Make BatchCommitRecords update the index in one request -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #135963|0 |1 is obsolete| | Attachment #135964|0 |1 is obsolete| | --- Comment #12 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 136218 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=136218&action=edit Bug 30822: Make BatchCommitRecords update the index in one request When committing staged marc imports to the catalogue we will often be importing a batch of records. We don't want to send one index request per biblio affected, we want to index them all after the records have been modified otherwise we will end up with multiple tasks per record (when items are also affected). Test plan: 1) Use the stage marc record tool to stage and commit a set of records and confirm the behaviour remains correct. 2) If using Elastic, check that only one indexing job is queued to take place resulting from the committed import. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 --- Comment #13 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 136219 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=136219&action=edit Bug 30822: Clarify that BatchCommitItems is a private function BatchCommitItems is only being used within this module and isn't mentioned in EXPORT_OK. This patch simply renames it to _batchCommitItems to take the _ standard for private functions and also adds a little hint to the POD of the function to clarify that the caller must trigger a re-index. JK: Amended patch to rename also the function in t/db_dependent/ImportBatch.t and fix typo "commiting" => "commiting" Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl Status|Signed Off |BLOCKED --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QA: Looking here -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Martin Renvoize from comment #6)
(In reply to Joonas Kylmälä from comment #5)
Good improvement! Just a small nit: as BatchCommitItems is a public function it should take either skip_record_index as a parameter or document clearly that it doesn't do indexing and it is left to the caller.
Good Catch..
I'm wondering if the third option is to rename the function to be 'private'.. it appears to only be called inside this module and isn't exported either. Certainly some POD around it would also be sensible though.
Just a thought. Not sure if we should invest time in renaming C4 functions private rather than getting them out of C4 ;) There will be a bunch more. Wouldnt rename them all. This is done, no problem. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 --- Comment #16 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- There is something funny going on here. Might be a config issue, but while the worker is running, I dont see any import finished but constantly a new process is spinning up for the background-job-progress ? Testing with Zebra only. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #10)
But maybe we need that part, for biblio deletions, what do you think?
+ if ( $record_type eq 'biblio' && ( @updated_biblionumbers || @deleted_biblionumbers ) ) { + my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX }); + if ( @deleted_biblionumbers ) { + $indexer->index_records( \@deleted_biblionumbers, "recordDelete", "biblioserver" ); + } else { + $indexer->index_records( \@updated_biblionumbers, "specialUpdate", "biblioserver" ); + } + }
I agree that for consistency, we should address that too. But we could say that this can be done on a new report. The title of this report is scoped to BatchCommitRecords. And we are talking BatchRevert here. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Marcel de Rooy from comment #16)
There is something funny going on here. Might be a config issue, but while the worker is running, I dont see any import finished but constantly a new process is spinning up for the background-job-progress ? Testing with Zebra only.
The worker is not yet relevant here. But what needs attention (somewhere), is that if BatchCommit silently fails, the js on the manage import page keeps polling for the status of something that crashed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|BLOCKED |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #136218|0 |1 is obsolete| | --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 136247 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=136247&action=edit Bug 30822: Make BatchCommitRecords update the index in one request When committing staged marc imports to the catalogue we will often be importing a batch of records. We don't want to send one index request per biblio affected, we want to index them all after the records have been modified otherwise we will end up with multiple tasks per record (when items are also affected). Test plan: 1) Use the stage marc record tool to stage and commit a set of records and confirm the behaviour remains correct. 2) If using Elastic, check that only one indexing job is queued to take place resulting from the committed import. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #136219|0 |1 is obsolete| | --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 136248 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=136248&action=edit Bug 30822: Clarify that BatchCommitItems is a private function BatchCommitItems is only being used within this module and isn't mentioned in EXPORT_OK. This patch simply renames it to _batchCommitItems to take the _ standard for private functions and also adds a little hint to the POD of the function to clarify that the caller must trigger a re-index. JK: Amended patch to rename also the function in t/db_dependent/ImportBatch.t and fix typo "commiting" => "commiting" Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 --- Comment #21 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Thanks guys, sorry I didn't get back to this one. Yes, I can take care of batchRevert next unless someone else wants to jump on that one. As for other failures I'm wondering if to some extent that's looked at in Bug 29325 - commit_file.pl error 'Already in a transaction'? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |normal -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |22.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 --- Comment #22 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 22.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #23 from Lucas Gass <lucas@bywatersolutions.com> --- Does not apply cleanly to 22.05.x, no backport. Please rebase if needed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |33019 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33019 [Bug 33019] Records not indexed in Elasticsearch ES when first catalogued -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30822 --- Comment #24 from Nick Clemens <nick@bywatersolutions.com> --- *** Bug 26543 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org