Bug 22690 - Merging records with many items too slow (Elasticsearch)
Summary: Merging records with many items too slow (Elasticsearch)
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Ere Maijala
QA Contact: Josef Moravec
URL:
Keywords:
Depends on:
Blocks: 20447
  Show dependency treegraph
 
Reported: 2019-04-11 14:19 UTC by Vitor Fernandes
Modified: 2021-02-11 09:56 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 22690 - Merging records with many items (ElasticSearch) (5.36 KB, patch)
2019-05-24 15:27 UTC, axel Amghar
Details | Diff | Splinter Review
Bug 22690: Merging records with many items (ElasticSearch) (5.16 KB, patch)
2019-05-27 11:28 UTC, axel Amghar
Details | Diff | Splinter Review
Bug 22690: Merging records with many items (ElasticSearch) (5.12 KB, patch)
2019-05-27 11:34 UTC, axel Amghar
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (16.57 KB, patch)
2019-09-16 11:16 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (16.23 KB, patch)
2019-10-22 07:09 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (16.23 KB, patch)
2020-01-22 09:59 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (16.30 KB, patch)
2020-01-30 09:57 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (16.28 KB, patch)
2020-01-31 07:43 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (16.29 KB, patch)
2020-01-31 07:50 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (16.71 KB, patch)
2020-02-21 13:30 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (16.66 KB, patch)
2020-02-21 13:37 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (16.72 KB, patch)
2020-03-08 21:41 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (16.95 KB, patch)
2020-05-12 17:22 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (17.01 KB, patch)
2020-05-12 17:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (17.47 KB, patch)
2020-05-14 07:05 UTC, Ere Maijala
Details | Diff | Splinter Review
error (45.23 KB, text/html)
2020-05-31 15:59 UTC, Victor Grousset/tuxayo
Details
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (17.50 KB, patch)
2020-06-22 07:09 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (17.34 KB, patch)
2020-08-21 07:19 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (17.25 KB, patch)
2020-09-07 10:17 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (17.78 KB, patch)
2020-09-30 13:37 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (17.81 KB, patch)
2020-09-30 14:43 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 22690 - Output from tests after patch applied (29.75 KB, text/plain)
2020-10-04 02:39 UTC, David Nind
Details
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (17.81 KB, patch)
2020-10-05 05:49 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 22690 - Output from tests for Item.t (14.72 KB, text/plain)
2020-10-05 13:29 UTC, David Nind
Details
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (17.87 KB, patch)
2020-10-05 14:04 UTC, David Nind
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (25.40 KB, patch)
2020-10-21 13:04 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (25.21 KB, patch)
2020-10-22 12:38 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (25.25 KB, patch)
2020-10-27 13:04 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (25.25 KB, patch)
2020-10-27 13:21 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Test results - Bug 22690 (52.19 KB, text/plain)
2020-10-30 20:03 UTC, David Nind
Details
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (25.23 KB, patch)
2020-11-09 12:31 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 22690: Add more tests (5.28 KB, patch)
2020-11-09 12:31 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (24.91 KB, patch)
2020-11-24 13:09 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 22690: Add more tests (5.28 KB, patch)
2020-11-24 13:09 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 22690: Refactor merging of records to improve performance (Elasticsearch) (24.98 KB, patch)
2020-11-24 21:29 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 22690: Add more tests (5.34 KB, patch)
2020-11-24 21:29 UTC, Michal Denar
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Vitor Fernandes 2019-04-11 14:19:26 UTC
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)
Comment 1 axel Amghar 2019-05-24 07:36:59 UTC
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?
Comment 2 Vitor Fernandes 2019-05-24 09:51:17 UTC
(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).
Comment 3 axel Amghar 2019-05-24 10:01:53 UTC
(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 ?
Comment 4 axel Amghar 2019-05-24 15:27:57 UTC Comment hidden (obsolete)
Comment 5 axel Amghar 2019-05-27 11:28:45 UTC Comment hidden (obsolete)
Comment 6 axel Amghar 2019-05-27 11:34:40 UTC
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
Comment 7 axel Amghar 2019-05-27 11:42:08 UTC
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?
Comment 8 Ere Maijala 2019-07-29 08:28:04 UTC
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.
Comment 9 Ere Maijala 2019-09-12 07:14:38 UTC
Taking this since I need this for bug 20447.
Comment 10 Ere Maijala 2019-09-16 11:16:10 UTC
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
Comment 11 Ere Maijala 2019-10-22 07:09:58 UTC
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
Comment 12 Joonas Kylmälä 2019-12-20 13:38:24 UTC
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)
Comment 13 Ere Maijala 2020-01-22 09:59:39 UTC
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
Comment 14 Ere Maijala 2020-01-22 10:00:00 UTC
Rebased.
Comment 15 Michal Denar 2020-01-25 16:12:32 UTC
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.
Comment 16 Ere Maijala 2020-01-27 08:10:10 UTC
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).
Comment 17 Michal Denar 2020-01-27 20:11:51 UTC
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?
Comment 18 Ere Maijala 2020-01-30 09:11:15 UTC
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?
Comment 19 Michal Denar 2020-01-30 09:31:08 UTC
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.
Comment 20 Michal Denar 2020-01-30 09:57:04 UTC
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 21 Martin Renvoize 2020-01-30 15:27:59 UTC
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?
Comment 22 Joonas Kylmälä 2020-01-31 06:46:04 UTC
(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.
Comment 23 Joonas Kylmälä 2020-01-31 06:59:28 UTC
(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?
Comment 24 Katrin Fischer 2020-01-31 07:10:57 UTC
> 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.
Comment 25 Joonas Kylmälä 2020-01-31 07:14:28 UTC
(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.
Comment 26 Ere Maijala 2020-01-31 07:23:52 UTC
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.
Comment 27 Ere Maijala 2020-01-31 07:43:18 UTC
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>
Comment 28 Ere Maijala 2020-01-31 07:50:12 UTC
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>
Comment 29 Ere Maijala 2020-01-31 07:50:50 UTC
Rename done.
Comment 30 Kyle M Hall 2020-02-20 16:41:54 UTC
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
Comment 31 Ere Maijala 2020-02-21 13:30:03 UTC
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>
Comment 32 Ere Maijala 2020-02-21 13:30:47 UTC
Fixed tests (use the new params for AddReserve).
Comment 33 Ere Maijala 2020-02-21 13:37:28 UTC
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
Comment 34 Michal Denar 2020-03-04 22:08:03 UTC
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
Comment 35 Ere Maijala 2020-03-05 09:07:14 UTC
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?
Comment 36 Michal Denar 2020-03-08 21:41:00 UTC
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>
Comment 37 Josef Moravec 2020-05-12 17:22:33 UTC
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>
Comment 38 Josef Moravec 2020-05-12 17:22:59 UTC
Just rebased on master
Comment 39 Josef Moravec 2020-05-12 17:27:09 UTC
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 40 Josef Moravec 2020-05-12 17:40:05 UTC
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)
Comment 41 Ere Maijala 2020-05-14 07:05:43 UTC
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>
Comment 42 Ere Maijala 2020-05-14 07:06:17 UTC
Thanks, Josef. Should be fixed now.
Comment 43 Victor Grousset/tuxayo 2020-05-31 15:59:58 UTC
Created attachment 105445 [details]
error

> With the patch:
> - Do the same as above
> - This time verify that the records are merged without timeout
Comment 44 Victor Grousset/tuxayo 2020-05-31 16:08:25 UTC
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.
Comment 45 Victor Grousset/tuxayo 2020-05-31 16:28:45 UTC
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
Comment 46 Victor Grousset/tuxayo 2020-05-31 17:11:25 UTC
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.
Comment 47 Victor Grousset/tuxayo 2020-05-31 17:40:43 UTC
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)
Comment 48 Victor Grousset/tuxayo 2020-05-31 17:44:57 UTC
One last thing, it also happens with ES5
Comment 49 Ere Maijala 2020-06-18 07:35:48 UTC
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.
Comment 50 Ere Maijala 2020-06-18 13:55:00 UTC
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.
Comment 51 Ere Maijala 2020-06-22 07:09:52 UTC
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>
Comment 52 Ere Maijala 2020-06-22 07:11:02 UTC
Oops. It _was_ still related to ES since I forgot to skip ES update when storing a single item. Should be fixed now.
Comment 53 Joonas Kylmälä 2020-08-13 14:39:54 UTC
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?
Comment 54 Joonas Kylmälä 2020-08-21 07:19:20 UTC
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>
Comment 55 Joonas Kylmälä 2020-09-07 10:17:01 UTC
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>
Comment 56 Joonas Kylmälä 2020-09-30 13:37:02 UTC
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>
Comment 57 Joonas Kylmälä 2020-09-30 14:43:22 UTC
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>
Comment 58 Joonas Kylmälä 2020-09-30 14:55:36 UTC
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.
Comment 59 David Nind 2020-10-03 08:25:20 UTC
Is there an easy way to add 1,000 items for a record?
Comment 60 Michal Denar 2020-10-03 16:43:21 UTC
Hi David,
yes, you can use New item/Add multiple copies of this item.
Comment 61 David Nind 2020-10-03 20:25:19 UTC
(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!
Comment 62 David Nind 2020-10-04 02:39:46 UTC
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!
Comment 63 Ere Maijala 2020-10-05 05:43:25 UTC
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.
Comment 64 Ere Maijala 2020-10-05 05:49:57 UTC
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>
Comment 65 Ere Maijala 2020-10-05 06:01:07 UTC
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.
Comment 66 David Nind 2020-10-05 13:29:35 UTC
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.
Comment 67 Joonas Kylmälä 2020-10-05 13:40:59 UTC
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).
Comment 68 David Nind 2020-10-05 14:04:59 UTC
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>
Comment 69 Katrin Fischer 2020-10-14 22:43:52 UTC
Hi Josef, I see you are QA contact on this one, will you be able to finish here or should we reassign?
Comment 70 Nick Clemens 2020-10-15 12:33:29 UTC
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)
Comment 71 Ere Maijala 2020-10-21 13:04:29 UTC
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>
Comment 72 Ere Maijala 2020-10-21 13:05:16 UTC
Hi Nick, and thanks for the excellent feedback. I've now implemented the changes you requested.
Comment 73 Ere Maijala 2020-10-22 12:38:15 UTC
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>
Comment 74 Ere Maijala 2020-10-22 12:39:20 UTC
Latest version makes sure params is defined in Item->move_to_biblio.
Comment 75 Joonas Kylmälä 2020-10-26 17:32:56 UTC
Patch doesn't apply anymore.
Comment 76 Joonas Kylmälä 2020-10-27 13:04:29 UTC
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>
Comment 77 Joonas Kylmälä 2020-10-27 13:06:09 UTC
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.
Comment 78 Joonas Kylmälä 2020-10-27 13:21:24 UTC
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>
Comment 79 Joonas Kylmälä 2020-10-27 13:24:44 UTC
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.
Comment 80 David Nind 2020-10-27 19:39:03 UTC
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.
Comment 81 Joonas Kylmälä 2020-10-28 08:20:37 UTC
(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?
Comment 82 David Nind 2020-10-30 20:03:12 UTC
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
Comment 83 Joonas Kylmälä 2020-11-02 07:45:00 UTC
(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.
Comment 84 Martin Renvoize 2020-11-02 15:16:19 UTC
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
Comment 85 Ere Maijala 2020-11-09 12:31:21 UTC
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>
Comment 86 Ere Maijala 2020-11-09 12:31:27 UTC
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
Comment 87 Ere Maijala 2020-11-09 12:32:08 UTC
(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.
Comment 88 Martin Renvoize 2020-11-09 13:03:01 UTC
Thanks Ere, I'll get back to QA on this asap :)
Comment 89 Joonas Kylmälä 2020-11-24 13:02:40 UTC
Doesn't apply anymore
Comment 90 Joonas Kylmälä 2020-11-24 13:09:16 UTC
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>
Comment 91 Joonas Kylmälä 2020-11-24 13:09:21 UTC
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
Comment 92 Michal Denar 2020-11-24 21:29:20 UTC
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>
Comment 93 Michal Denar 2020-11-24 21:29:25 UTC
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>
Comment 94 Jonathan Druart 2021-02-11 09:48:46 UTC
So the problem is actually that the reindex is not async.
Bug 27344 is trying to provide a solution for that.
Comment 95 Ere Maijala 2021-02-11 09:56:37 UTC
(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.