When merging records with many items, Koha reindexes the origin record and the destination record for each item moving (check MoveItemFromBiblio in C4::Items). If the record being eliminated has 1000 items, Koha will make 2000 reindexes. Because of this behaviour, the merging is too slow. Test plan: - Create 2 records (one with 1000 items for example) - Add the records to one list and start the merging process of those records. - Choose the record with many items as the one to be eliminated. - Start the merging - After a while the web server should give you a timeout error (the merging process continues)
hello, when i try to merge records, i have an issue : Following required fields are missing: -801 https://snag.gy/76Fh9R.jpg this is a mandatory tag but i can't add it Do someone know why?
(In reply to axel Amghar from comment #1) > hello, > when i try to merge records, i have an issue : > Following required fields are missing: > -801 > > > https://snag.gy/76Fh9R.jpg > this is a mandatory tag but i can't add it > Do someone know why? Axel that happens because the framework chosen for merging has 801 as mandatory field (that's my guess).
(In reply to Vitor Fernandes from comment #2) > Axel that happens because the framework chosen for merging has 801 as > mandatory field (that's my guess). I think is the same thing with "000" and "001". They are mandatory but we can't add them. So.. Do we have to fix it ?
Created attachment 90061 [details] [review] Bug 22690 - Merging records with many items (ElasticSearch) This patch allow us to merged many items without timeout Test plan : Without the patch : > - Create 2 records (one with 1000 items for example) > - Add the records to one list and start the merging process of those records. > - Choose the record with many items as the one to be eliminated. > - Start the merging > - After a while the web server should give you a timeout error (the merging process continues) With the patch : - make the same - this time verify that all items have been merged without time out
Created attachment 90115 [details] [review] Bug 22690: Merging records with many items (ElasticSearch) This patch allow us to merged many items without timeout Test plan : Without the patch : > - Create 2 records (one with 1000 items for example) > - Add the records to one list and start the merging process of those records. > - Choose the record with many items as the one to be eliminated. > - Start the merging > - After a while the web server should give you a timeout error (the merging process continues) With the patch : - make the same - this time verify that all items have been merged without time out
Created attachment 90116 [details] [review] Bug 22690: Merging records with many items (ElasticSearch) This patch allow us to merged many items without timeout Test plan : Without the patch : > - Create 2 records (one with 1000 items for example) > - Add the records to one list and start the merging process of those records. > - Choose the record with many items as the one to be eliminated. > - Start the merging > - After a while the web server should give you a timeout error (the merging process continues) With the patch : - make the same - this time verify that all items have been merged without time out
Hello, I found an issue when you click on merge selected, and then you click instant on merge, the destination record doesn't have the time to load. And when you look your record, it lose his MARC record. Someone else succeed to reproduce this bug?
I don't get the logic of the $items_number parameter. As far as I can see the first UPDATE moves all items regardless of the parameter, but the subsequent processing of acquisitions etc. only goes through items in the $items_number parameter. If MoveItemsFromBiblio is supposed to move all items, I'd remove the $items_number parameter and do everything necessary inside MoveItemsFromBiblio. Also, I believe the 801 issue should be handled in a separate bug since it's not related to Elasticsearch.
Taking this since I need this for bug 20447.
Created attachment 92828 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t
Created attachment 94505 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t
Doesn't apply anymore: Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 22690: Refactor merging of records to improve performance (Elasticsearch) Using index info to reconstruct a base tree... M C4/Items.pm M Koha/Biblio.pm M Koha/Item.pm M t/db_dependent/Koha/Item.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Koha/Item.t Auto-merging Koha/Item.pm CONFLICT (content): Merge conflict in Koha/Item.pm Auto-merging Koha/Biblio.pm CONFLICT (content): Merge conflict in Koha/Biblio.pm Auto-merging C4/Items.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 22690: Refactor merging of records to improve performance (Elasticsearch)
Created attachment 97696 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t
Rebased.
Hi Ere, patch working, but one test fails: Test Summary Report ------------------- t/db_dependent/Koha/Item.t (Wstat: 256 Tests: 5 Failed: 1) Failed test: 3 Non-zero exit status: 1 Second test was OK.
Mike, which test is failing? I'm seeing some errors from an unrelated test that tries to clean up the database in wrong order (remove patrons before their checkouts).
Hi Ere, prove t/db_dependent/Koha/Item.t: not ok 3 - Value is mapped correctly for column biblionumber # Failed test 'Value is mapped correctly for column biblionumber' # at t/db_dependent/Koha/Item.t line 109. # got: undef # expected: '462' not ok 4 - Value is mapped correctly for column biblioitemnumber # Failed test 'Value is mapped correctly for column biblioitemnumber' # at t/db_dependent/Koha/Item.t line 109. # got: undef # expected: '461' not ok 28 - Value is mapped correctly for column timestamp # Failed test 'Value is mapped correctly for column timestamp' # at t/db_dependent/Koha/Item.t line 109. # got: undef # expected: '2020-01-24 22:23:22' not ok 42 - Value is mapped correctly for column biblionumber # Failed test 'Value is mapped correctly for column biblionumber' # at t/db_dependent/Koha/Item.t line 124. # got: undef # expected: '462' not ok 67 - Value is mapped correctly for column timestamp # Failed test 'Value is mapped correctly for column timestamp' # at t/db_dependent/Koha/Item.t line 124. # got: undef # expected: '2020-01-24 22:23:22' # Looks like you failed 6 tests of 79. not ok 3 - as_marc_field() tests It's problem with my mappings?
Mike, that seems unrelated to any of the changes here. I tried with both MARC 21 and UNIMARC default mappings and couldn't reproduce, so I'd say it's an issue with your mappings, but I'm a bit at loss if they're at defaults. Do you see the issue with plain master version of Koha?
Hi Ere, I've kohadevbox on master, with almost defaults settings. Framework test show 1 error: subfields not in same tabs, but at biblio fileds. I'll test it again, because and propably switch to SO.
Created attachment 98143 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t Signed-off-by: Michal Denar <black23@gmail.com>
Comment on attachment 98143 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) Review of attachment 98143 [details] [review]: ----------------------------------------------------------------- This looks like a reasonable approach to me and seems to work well.. a relatively minor point regarding the introduce Koha/Biblio method. ::: Koha/Biblio.pm @@ +854,5 @@ > +Move items from the given biblio > + > +=cut > + > +sub move_items_from_biblio { I feel like 'move_items_from_biblio' isn't immediately obvious as a function name.. are we moving from 'this' biblio or to it.. perhaps 'adopt_items_from_biblio' is a bit clearer?
(In reply to Martin Renvoize from comment #21) > Comment on attachment 98143 [details] [review] [review] > Bug 22690: Refactor merging of records to improve performance (Elasticsearch) > > Review of attachment 98143 [details] [review] [review]: > ----------------------------------------------------------------- > > This looks like a reasonable approach to me and seems to work well.. a > relatively minor point regarding the introduce Koha/Biblio method. > > ::: Koha/Biblio.pm > @@ +854,5 @@ > > +Move items from the given biblio > > + > > +=cut > > + > > +sub move_items_from_biblio { > > I feel like 'move_items_from_biblio' isn't immediately obvious as a function > name.. are we moving from 'this' biblio or to it.. perhaps > 'adopt_items_from_biblio' is a bit clearer? I would just reverse the original idea: move_items_to_biblio. Then it should be obvious we are talking about the current object's items being moved. Not sure if perl OO style supports overloading, but in that case it could be just move_items, and the parameters defined whether we move them to another biblio or (yet to be included to Koha) to a holdings record.
(In reply to Joonas Kylmälä from comment #22) > I would just reverse the original idea: move_items_to_biblio. Then it should > be obvious we are talking about the current object's items being moved. Not > sure if perl OO style supports overloading, but in that case it could be > just move_items, and the parameters defined whether we move them to another > biblio or (yet to be included to Koha) to a holdings record. Or we could do this even in the Item and Items objects for maximum code reusability, like $biblio->items->move_to_biblio?
> Or we could do this even in the Item and Items objects for maximum code > reusability, like $biblio->items->move_to_biblio? Maybe just move in this case? They can't go anywhere else than another biblio.
(In reply to Katrin Fischer from comment #24) > > Or we could do this even in the Item and Items objects for maximum code > > reusability, like $biblio->items->move_to_biblio? > > Maybe just move in this case? They can't go anywhere else than another > biblio. This bug is a dependency for Bug 20447 which introduces holdings records so similar moving of items to another holdings record is needed there in addition to moving to another biblio.
I'll rename the method as Martin suggested. Any other changes will cause complications with bug 20447 where we need to move holdings too (so that would become adopt_holdings_and_items_from_biblio). They can't really be separated since they depend on each other.
Created attachment 98210 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 98211 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t Signed-off-by: Michal Denar <black23@gmail.com>
Rename done.
root@6d3e2c82bf14:koha(bug22690-qa)$ prove t/db_dependent/Koha/Item.t t/db_dependent/Koha/Item.t .. 4/5 # No tests run! t/db_dependent/Koha/Item.t .. 5/5 # Failed test 'No tests run for subtest "move_to_biblio() tests"' # at t/db_dependent/Koha/Item.t line 435. Can't use string ("k8XjSy") as a HASH ref while "strict refs" in use at /kohadevbox/koha/C4/Reserves.pm line 175. # Looks like your test exited with 255 just after 5. t/db_dependent/Koha/Item.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 1/5 subtests Test Summary Report ------------------- t/db_dependent/Koha/Item.t (Wstat: 65280 Tests: 5 Failed: 1) Failed test: 5 Non-zero exit status: 255 Files=1, Tests=5, 6 wallclock secs ( 0.04 usr 0.01 sys + 3.30 cusr 0.62 csys = 3.97 CPU) Result: FAIL
Created attachment 99376 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t Signed-off-by: Michal Denar <black23@gmail.com>
Fixed tests (use the new params for AddReserve).
Created attachment 99379 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t
Hi Ere, I get some errors for prove -v t/db_dependent/Koha/Item.t # Looks like you failed 6 tests of 79. not ok 3 - as_marc_field() tests ok 5 - move_to_biblio() tests # Looks like you failed 1 test of 5. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/5 subtests Test Summary Report ------------------- t/db_dependent/Koha/Item.t (Wstat: 256 Tests: 5 Failed: 1) Failed test: 3 Non-zero exit status: 1 Files=1, Tests=5, 4 wallclock secs ( 0.03 usr 0.02 sys + 2.70 cusr 0.32 csys = 3.07 CPU) Result: FAIL
Mike, I can't reproduce. Can you provide more insight on which tests failed? And is it an empty database or does it have existing data?
Created attachment 100316 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 104796 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t Signed-off-by: Michal Denar <black23@gmail.com>
Just rebased on master
Created attachment 104797 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t Signed-off-by: Michal Denar <black23@gmail.com>
Comment on attachment 104797 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) Review of attachment 104797 [details] [review]: ----------------------------------------------------------------- Great job Ere, I have just few concerns about the code. But I like this refactor a lot! ::: Koha/Item.pm @@ +840,5 @@ > + > + my $schema = Koha::Database->new()->schema(); > + > + # Acquisition orders related to the item > + my $orders = $schema->resultset('Aqorder')->search( Koha::Object(s) should be used @@ +847,5 @@ > + ); > + $orders->update_all({ biblionumber => $biblionumber }); > + > + # reserves > + my $reserves = $self->_result->reserves; there is $self->holds method @@ +867,5 @@ > + $dbh->do("UPDATE tmp_holdsqueue SET biblionumber=? WHERE itemnumber=?", undef, $biblionumber, $self->itemnumber); > + } > + ); > + #my $tmp_holdsqueues = $self->_result->tmp_holdsqueues; > + #$tmp_holdsqueues->update_all({ biblionumber => $biblionumber }); Please, remove these commented lines ::: cataloguing/merge.pl @@ +106,5 @@ > UPDATE suggestions SET biblionumber = ? WHERE biblionumber = ? > "); > + my $sth_articlerequests = $dbh->prepare(" > + UPDATE article_requests SET biblionumber = ? WHERE biblionumber = ? > + "); There should be no SQL in .pl script. Please, use Koha::Object(s)
Created attachment 104871 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t Signed-off-by: Michal Denar <black23@gmail.com>
Thanks, Josef. Should be fixed now.
Created attachment 105445 [details] error > With the patch: > - Do the same as above > - This time verify that the records are merged without timeout
BTW, librarian question: is it expected to for the merge to complain about the 003 field being missing? (and blocking the merge). So in my test data (koha-testing-docker and I supose KohaDevBox), I have to edit the books framework to make 003 visible so it will be populated when I create the two records.
The failure in comment 43 might have been because I had the patch applied before initialize my dev env (koha-testing-docker). So retried: - initialize & start instance - apply patch - restart_all - try the merge - timeout So there is definitely an issue
The above tests where done on Debian with MariaDB 10.3 with ElasticSearch 6.8.8 == == I retried the above simplified test plan (second half of the full one) with Debian 9, MySQL 5.5 and still ES 6.8.8 Still the same issue. Though Koha is stuck on additem.pl whereas in the previous test it's merge.pl that I was seeing the process list to. (when checking that it was still running in the background despite the timeout) == == I retried what I did on comment 43 on Debian 9& MySQL 5.5 and it didn't happen (no error, stuck on additem.pl). So maybe it's linked to the OS & DB or I didn't something else that messed it up. == == I think that's all for now from me. I'll be happy to do some more tests with a new patch or precises instructions to help troubleshoot the issue in case it only happens to me.
Forget what I said about being stuck on additem.pl or merge.pl, it seems related to the first that I queried after I reset the dev env. After I do a "reset_all", the process that is stuck at 100% CPU is the starman worker. (so maybe it was some kind of other bug)
One last thing, it also happens with ES5
Victor, the patch doesn't change anything with the framework handling, so whether that's a problem is unrelated to this issue, as far as I can see. Thanks for testing in any case. I'll see if I can reproduce the problem.
Ok, I can see it's not fast enough, but it doesn't seem to be related to Elasticsearch but the fact that Koha::Objects must be used. I'll see what I can do to improve it.
Created attachment 106144 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t Signed-off-by: Michal Denar <black23@gmail.com>
Oops. It _was_ still related to ES since I forgot to skip ES update when storing a single item. Should be fixed now.
Michal: Could you verify the patch is still signed-off even after the rebase Ere did? It seems like he forgot your sign-off there even though I think it should have been removed after the rebase. If the patch is signed-off could you change the bug status to Signed-off?
Created attachment 108777 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 109694 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 110991 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 110994 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Rebased to solve conflict caused by "Bug 25265: Prevent double reindex of the same item in batchmod" / 88cb7f223d. Replaced ModZebra calls with Koha::SearchEngine::Indexer.
Is there an easy way to add 1,000 items for a record?
Hi David, yes, you can use New item/Add multiple copies of this item.
(In reply to Michal Denar from comment #60) > Hi David, > yes, you can use New item/Add multiple copies of this item. Thanks Michal! I can see it now!
Created attachment 111196 [details] Bug 22690 - Output from tests after patch applied I had a go at testing this with koha-testing-docker on my average laptop (default KTD uses ES 6.x). After applying the patch the time reduced from about an hour to around 30 minutes for all the records to be merged and listed under the record (in the staff interface and OPAC). About 5 mins in I get this timeout message in the browser: Proxy Error The proxy server received an invalid response from an upstream server. The proxy server could not handle the request Reason: Error reading from remote server Apache/2.4.25 (Debian) Server at 127.0.0.1 Port 8081 Test results after patch applied: - prove -v t/db_dependent/Koha/Item.t - fail, took about 30 minutes to run - prove -v t/db_dependent/Items/MoveItemFromBiblio.t I haven't changed the status to failed QA. I'm not sure my testing has helped!
David, thanks for testing. It's very valuable! I'm seeing the same as you, and it's not good. I'll investigate why that is.
Created attachment 111207 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
There was a parameter name change in master that caused the biblio to be reindexed separately for every item. Now fixed, and you should see greatly improved performance.
Created attachment 111236 [details] Bug 22690 - Output from tests for Item.t Thanks Ere! That made a huge difference, and the merge takes almost no time at all now (less than 1 minute) However, the the tests for prove -v t/db_dependent/Koha/Item.t run a lot faster as well, but still fail - see attachment Bug 22690 - Output from tests for Item.t.
Hi, (In reply to David Nind from comment #66) > However, the the tests for prove -v t/db_dependent/Koha/Item.t run a lot > faster as well, but still fail - see attachment Bug 22690 - Output from > tests for Item.t. please run the test after running "reset_all" and it should work. The Koha unit tests tend to fail if you change system preferences (which seems to be the case here with the searchengine syspref).
Created attachment 111237 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: David Nind <david@davidnind.com>
Hi Josef, I see you are QA contact on this one, will you be able to finish here or should we reassign?
Hi Ere, Thank you! Great refactoring! A few things: 1 - Generally we assume that we should index unless passed 'skip_record_index', rather than hardcoding not to 2 - If you change 'move_to_biblio' to take that param and allow the store to index otherwise then we can can remove MoveItemFromBiblio and simply use move_to_biblio in moveitem.pl 3 - please add a test to t/db_dependent/Koha/SearchEngine/Indexer.t to cover when indexing happens (or doesn't) for these subroutines (to prove the adopt method only calls once)
Created attachment 112100 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Koha/SearchEngine/Indexer.t Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Hi Nick, and thanks for the excellent feedback. I've now implemented the changes you requested.
Created attachment 112184 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Koha/SearchEngine/Indexer.t Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Latest version makes sure params is defined in Item->move_to_biblio.
Patch doesn't apply anymore.
Created attachment 112567 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Koha/SearchEngine/Indexer.t Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Rebased. However the patch has two QA failures reported by the QA tool: FAIL C4/Items.pm OK critic OK forbidden patterns OK git manipulation OK pod FAIL pod coverage OK spelling OK valid FAIL cataloguing/moveitem.pl OK critic OK forbidden patterns OK git manipulation OK pod OK spelling FAIL valid These need to be fixed.
Created attachment 112569 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Koha/SearchEngine/Indexer.t Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Fixed the QA tool error FAIL cataloguing/moveitem.pl with > - my $to_biblio = Koha::Biblios->find($biblionumber) > + my $to_biblio = Koha::Biblios->find($biblionumber); Probably happened during one of the rebases. The POD coverage fail FAIL C4/Items.pm is not caused by caused by this patch as far as I can tell. Moving to Needs Signoff.
I'm getting test failures with prove -v t/db_dependent/Koha/Item.t after the patch is applied. I ran this after applying the patches, but before adding the 1000 items and merging. They passed before the patch was applied. Note: I couldn't get the tests to complete after adding the 1000 items and merging. Apart from this, everything else in the test plan works.
(In reply to David Nind from comment #80) > I'm getting test failures with prove -v t/db_dependent/Koha/Item.t after the > patch is applied. I ran this after applying the patches, but before adding > the 1000 items and merging. > > They passed before the patch was applied. > > Note: I couldn't get the tests to complete after adding the 1000 items and > merging. > > Apart from this, everything else in the test plan works. I cannot reproduce on koha-testing-docker, what is the error you get?
Created attachment 112754 [details] Test results - Bug 22690 Attached are the test results I got using koha-testing-docker: - Before the patch is applied: the tests pass - After changing the search engine system preference to elasticsearch and reindexing: tests fail - After patch applied: tests fail - After 1,000 records added and merging: tests fail
(In reply to David Nind from comment #82) > - Before the patch is applied: the tests pass > - After changing the search engine system preference to elasticsearch and > reindexing: tests fail > - After patch applied: tests fail > - After 1,000 records added and merging: tests fail This seems to be the same case as in comment 67.
I think the test failure is a red herring.. However.. I believe we need to add unit tests for the new Koha::Biblio->adopt_items_from_biblio method and Koha::Item->item_orders relation accessor before we proceed. These should be fairly trivial tests to add, one to prove that the loop happens and the final indexing takes place for the set and the second to prove the relation is setup correctly. Failing QA
Created attachment 113308 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Koha/SearchEngine/Indexer.t Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 113309 [details] [review] Bug 22690: Add more tests - Tests for adopt_items_from_biblio - Tests for the relationship between items and acquisition orders - Tests for indexer calls in adopt_items_from_biblio
(In reply to Martin Renvoize from comment #84) > However.. I believe we need to add unit tests for the new > Koha::Biblio->adopt_items_from_biblio method and Koha::Item->item_orders > relation accessor before we proceed. Absolutely. Tests now added.
Thanks Ere, I'll get back to QA on this asap :)
Doesn't apply anymore
Created attachment 113941 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Koha/SearchEngine/Indexer.t Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 113942 [details] [review] Bug 22690: Add more tests - Tests for adopt_items_from_biblio - Tests for the relationship between items and acquisition orders - Tests for indexer calls in adopt_items_from_biblio
Created attachment 113967 [details] [review] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Koha/SearchEngine/Indexer.t Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 113968 [details] [review] Bug 22690: Add more tests - Tests for adopt_items_from_biblio - Tests for the relationship between items and acquisition orders - Tests for indexer calls in adopt_items_from_biblio Signed-off-by: Michal Denar <black23@gmail.com>
So the problem is actually that the reindex is not async. Bug 27344 is trying to provide a solution for that.
(In reply to Jonathan Druart from comment #94) > So the problem is actually that the reindex is not async. > Bug 27344 is trying to provide a solution for that. That's part of it, but I believe the refactoring improves the flow and readability of the code as well.