[Bug 26581] New: Elasticsearch - Records can be indexed multiple times during returns
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 Bug ID: 26581 Summary: Elasticsearch - Records can be indexed multiple times during returns Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Searching - Zebra Assignee: koha-bugs@lists.koha-community.org Reporter: nick@bywatersolutions.com Throughout add return we actually make several updates to the item: We set onloan -> null We update item location if pref says so We update datelast seen We update not for loan is pref says so etc. For ES, this means a new connection and a reindex each time. This is problematic -- 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=26581 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- 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=26581 --- Comment #1 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 111001 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111001&action=edit Bug 26581: Unit tests These tests cover the changes to ensure the AddReturn calls index_records once per call and that other calls pass the skip_record_index parameter correctly -- 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=26581 --- Comment #2 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 111002 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111002&action=edit Bug 26581: Only reindex records once per checkin This patch simply passes skip_record_index calls to Koha:Item:store for all the changes done in AddReturn. Testing is really verifiying that items are still correctly indexed at the end For both search engines To test: 1 - Find or create a record with multipel items 2 - Populate both: UpdateItemLocationOnCheckin UpdateNotForLoanStatusOnCheckin 3 - Confirm that checking in an item correctly updates the item status in search results 4 - Test with items issued, and items not issued 5 - Test when generating a transfer (checkin at different branch) 6 - Test when item was marked lost 7 - Test when filling transfer -- 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=26581 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=25265 Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | -- 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=26581 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Shouldn't actually do the reverse and index on demand? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jonathan Druart from comment #3)
Shouldn't actually do the reverse and index on demand?
*we* Well, it's just a thought, I haven't investigated it! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 --- Comment #5 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #4)
(In reply to Jonathan Druart from comment #3)
Shouldn't actually do the reverse and index on demand?
*we*
Well, it's just a thought, I haven't investigated it!
Can you explain a bit more? Are you suggesting 'index_record' and don't index unless passed? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 --- Comment #6 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Nick Clemens from comment #5)
(In reply to Jonathan Druart from comment #4)
(In reply to Jonathan Druart from comment #3)
Shouldn't actually do the reverse and index on demand?
*we*
Well, it's just a thought, I haven't investigated it!
Can you explain a bit more? Are you suggesting 'index_record' and don't index unless passed?
Yes -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 --- Comment #7 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #6)
(In reply to Nick Clemens from comment #5)
(In reply to Jonathan Druart from comment #4)
(In reply to Jonathan Druart from comment #3)
Shouldn't actually do the reverse and index on demand?
*we*
Well, it's just a thought, I haven't investigated it!
Can you explain a bit more? Are you suggesting 'index_record' and don't index unless passed?
Yes
I am not opposed to the idea, however, it would mean redoing a lot of the code which was just done in bug 25265 and other places. I suspect it would not eliminate code, but just change the basic idea. Here we could possibly eliminate some of the storage calls, to reduce the number we have to 'skip' butlarger refactoring is needed overall so this was a start -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 Séverine Queune <severine.queune@bulac.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |severine.queune@bulac.fr --- Comment #8 from Séverine Queune <severine.queune@bulac.fr> --- I think I've tested all the different possible cases with ES and it works as expected, but I don't have Zebra running on devbox. Should I sign the patch thought ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 Andrew Fuerste-Henry <andrew@bywatersolutions.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=26581 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #111001|0 |1 is obsolete| | --- Comment #9 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 111596 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111596&action=edit Bug 26581: Unit tests These tests cover the changes to ensure the AddReturn calls index_records once per call and that other calls pass the skip_record_index parameter correctly Signed-off-by: Christoper Brannon <cbrannon@cdalibrary.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #111002|0 |1 is obsolete| | --- Comment #10 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 111597 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111597&action=edit Bug 26581: Only reindex records once per checkin This patch simply passes skip_record_index calls to Koha:Item:store for all the changes done in AddReturn. Testing is really verifiying that items are still correctly indexed at the end For both search engines To test: 1 - Find or create a record with multipel items 2 - Populate both: UpdateItemLocationOnCheckin UpdateNotForLoanStatusOnCheckin 3 - Confirm that checking in an item correctly updates the item status in search results 4 - Test with items issued, and items not issued 5 - Test when generating a transfer (checkin at different branch) 6 - Test when item was marked lost 7 - Test when filling transfer Signed-off-by: Christoper Brannon <cbrannon@cdalibrary.org
;
Signed-off-by: Christoper Brannon <cbrannon@cdalibrary.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_20_11_target -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #111596|0 |1 is obsolete| | --- Comment #11 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 111878 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111878&action=edit Bug 26581: Unit tests These tests cover the changes to ensure the AddReturn calls index_records once per call and that other calls pass the skip_record_index parameter correctly Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Christoper Brannon <cbrannon@cdalibrary.org> 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=26581 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #111597|0 |1 is obsolete| | --- Comment #12 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 111879 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111879&action=edit Bug 26581: Only reindex records once per checkin This patch simply passes skip_record_index calls to Koha:Item:store for all the changes done in AddReturn. Testing is really verifiying that items are still correctly indexed at the end For both search engines To test: 1 - Find or create a record with multipel items 2 - Populate both: UpdateItemLocationOnCheckin UpdateNotForLoanStatusOnCheckin 3 - Confirm that checking in an item correctly updates the item status in search results 4 - Test with items issued, and items not issued 5 - Test when generating a transfer (checkin at different branch) 6 - Test when item was marked lost 7 - Test when filling transfer Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Christoper Brannon <cbrannon@cdalibrary.org> 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=26581 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA CC| |martin.renvoize@ptfs-europe | |.com --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- This works as expected and passes all tests. I would really like to see us reduce the calls to store in the long run, rather than just adding a flag to prevent the re-indexing.. but this certainly does the trick for now and that's a longer term goal. Passing QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |20.11.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=26581 --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 20.11, 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=26581 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #15 from Lucas Gass <lucas@bywatersolutions.com> --- this patchset does not apply cleanly to 20.05.x, 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=26581 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_20_11_target |additional_work_needed --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- There are two failing tests caused by this change: t/db_dependent/SIP/Message.t .. 1/9 # Failed test 'OK flag is false when we check in at the wrong branch and we do not allow it' # at t/db_dependent/SIP/Message.t line 632. # got: '1' # expected: '0' # Failed test 'Code AF not found in '|AOz2h4hfYk|ABjPj5C_zEvKlqRRsSsLQf|AQz2h4hfYk|AJSome boring read|CKMHE|CTz2h4hfYk|CV04|'?' # at t/db_dependent/SIP/Message.t line 733. # got: '0' # expected: '1' # Looks like you failed 2 tests of 33. t/db_dependent/SIP/Message.t .. 3/9 # Failed test 'Checkin V2' # at t/db_dependent/SIP/Message.t line 76. Also maybe t/db_dependent/Koha/SearchEngine/Indexer.t that is failing on jenkins (but cannot recreate locally). Please fix asap. Also I don't understand what happened but the authorship of the first patch changed during the QA process. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 --- Comment #17 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 112563 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=112563&action=edit Bug 26581: [20.05.x] Only reindex records once per checkin This patch simply passes skip_record_index calls to Koha:Item:store for all the changes done in AddReturn. Testing is really verifiying that items are still correctly indexed at the end For both search engines To test: 1 - Find or create a record with multipel items 2 - Populate both: UpdateItemLocationOnCheckin UpdateNotForLoanStatusOnCheckin 3 - Confirm that checking in an item correctly updates the item status in search results 4 - Test with items issued, and items not issued 5 - Test when generating a transfer (checkin at different branch) 6 - Test when item was marked lost 7 - Test when filling transfer Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Christoper Brannon <cbrannon@cdalibrary.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Bug 26581: Unit tests These tests cover the changes to ensure the AddReturn calls index_records once per call and that other calls pass the skip_record_index parameter correctly Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Christoper Brannon <cbrannon@cdalibrary.org> 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=26581 --- Comment #18 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 112578 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=112578&action=edit Bug 26581: (follow-up) Restore unsetting of doreturn -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Nick Clemens from comment #18)
Created attachment 112578 [details] [review] Bug 26581: (follow-up) Restore unsetting of doreturn
Is this supposed to fix comment 16? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Nick Clemens from comment #18)
Created attachment 112578 [details] [review] Bug 26581: (follow-up) Restore unsetting of doreturn
Pushed to master for 20.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 --- Comment #21 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 112829 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=112829&action=edit Bug 26581: (follow-up) Skip merge when deleting authority Tests failing, seems to be because the indexes haven't been created yet. When deleting authority we perform a 'merge' where we search for biblios containing the authority. Since these are simply tets we can skip this step, avoid the search, and avoid a crash -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 --- Comment #22 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Nick Clemens from comment #21)
Created attachment 112829 [details] [review] Bug 26581: (follow-up) Skip merge when deleting authority
Tests failing, seems to be because the indexes haven't been created yet. When deleting authority we perform a 'merge' where we search for biblios containing the authority.
Since these are simply tets we can skip this step, avoid the search, and avoid a crash
Pushed to master, thanks Nick! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable Version(s)|20.11.00 |20.11.00, 20.05.06 released in| | --- Comment #23 from Lucas Gass <lucas@bywatersolutions.com> --- backported to 20.05.x for 20.05.06 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 Aleisha Amohia <aleisha@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |aleisha@catalyst.net.nz --- Comment #24 from Aleisha Amohia <aleisha@catalyst.net.nz> --- does not apply cleanly on 19.11.x, please rebase -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 --- Comment #25 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Aleisha Amohia from comment #24)
does not apply cleanly on 19.11.x, please rebase
Relies on 25265 which relies on 26507 (though not really) which relies on 23463. Changes a bit bigger than I can clear through easily, anyone else is welcome to take stab, but I think it's okay if only in stable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 Aleisha Amohia <aleisha@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |25265 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25265 [Bug 25265] Elasticsearch - Batch editing items on a biblio can lead to incorrect index -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 --- Comment #26 from Aleisha Amohia <aleisha@catalyst.net.nz> --- (In reply to Nick Clemens from comment #25)
(In reply to Aleisha Amohia from comment #24)
does not apply cleanly on 19.11.x, please rebase
Relies on 25265 which relies on 26507 (though not really) which relies on 23463.
Thanks, I have added the dependency to the bug report. missing dependencies, not backported to 19.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26581 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |28472 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28472 [Bug 28472] UpdateItemLocationOnCheckin not updating items where location is null -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org