Bug 28268 - Improve memory usage when indexing authorities in Elasticsearch
Summary: Improve memory usage when indexing authorities in Elasticsearch
Status: CLOSED FIXED
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:
URL: https://lists.katipo.co.nz/pipermail/...
Keywords:
Depends on:
Blocks:
 
Reported: 2021-04-30 06:53 UTC by Ere Maijala
Modified: 2021-12-13 21:11 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.06


Attachments
Bug 28268: Improve memory usage when indexing authorities in Elasticsearch (2.62 KB, patch)
2021-04-30 08:45 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 28268: Improve memory usage when indexing authorities in Elasticsearch (3.00 KB, patch)
2021-04-30 08:56 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 28268: Improve memory usage when indexing authorities in Elasticsearch (3.07 KB, patch)
2021-05-04 03:32 UTC, Aleisha Amohia
Details | Diff | Splinter Review
performance comparison (4.88 KB, text/plain)
2021-05-12 12:31 UTC, Nick Clemens
Details
Bug 28268: Improve memory usage when indexing authorities in Elasticsearch (3.12 KB, patch)
2021-05-12 12:54 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ere Maijala 2021-04-30 06:53:04 UTC
Currently indexing authorities in Elasticsearch creates a record set of all reocrds that includes the MARCXML records. This can use a huge amount of memory and prevent the indexing altogether. We should do the same as we do with biblios in that the full record set only contains the authority id's.

Further on, it would make sense to process the record in similar way to bug 27584, but that's another issue.
Comment 1 Jonathan Druart 2021-04-30 07:22:29 UTC
I am not sure I understand. It's an iterator, we are not going to load all the records in memory. The max we expect is the --commit param passed to the script (default 5000).

Waiting for your patch :)
Comment 2 Ere Maijala 2021-04-30 08:45:16 UTC Comment hidden (obsolete)
Comment 3 Ere Maijala 2021-04-30 08:56:14 UTC
Created attachment 120330 [details] [review]
Bug 28268: Improve memory usage when indexing authorities in Elasticsearch

Retrieves the complete records one by one to avoid huge memory usage.

Note that this removes the call to GuessAuthTypeCode, but it is done later in Koha::SearchEngine::Elasticsearch::marc_records_to_documents (and was never done if you asked to index a single record with --authid parameter).

Test plan:
1. Apply patch
2. Reindex authorities: perl misc/search_tools/rebuild_elasticsearch.pl -a -d -v
3. Check that indexing completed successfully and results are correct.
Comment 4 Ere Maijala 2021-04-30 09:05:41 UTC
This patch changes the mechanism so that the callback used to retrieve a single record retrieves authtypecode and marcxml fields. Well, actually it delegates the work to get_from_authid. As a result the record set retrieved from database only contains authid, which consumes quite a bit less memory than all the fields together. This also mirrors the method used in get_all_biblios_iterator.

Surprisingly this seems to be also faster than the old mechanism.
Comment 5 Aleisha Amohia 2021-05-04 03:32:00 UTC
Created attachment 120424 [details] [review]
Bug 28268: Improve memory usage when indexing authorities in Elasticsearch

Retrieves the complete records one by one to avoid huge memory usage.

Note that this removes the call to GuessAuthTypeCode, but it is done later in Koha::SearchEngine::Elasticsearch::marc_records_to_documents (and was never done if you asked to index a single record with --authid parameter).

Test plan:
1. Apply patch
2. Reindex authorities: perl misc/search_tools/rebuild_elasticsearch.pl -a -d -v
3. Check that indexing completed successfully and results are correct.

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 6 Nick Clemens 2021-05-12 12:31:22 UTC
Created attachment 120865 [details]
performance comparison

Three runs before and after the patch, 53927 auth records - machine generated from about ~30k bibs

Command I ran:
/usr/bin/time -v perl misc/search_tools/rebuild_elasticsearch.pl -v -d -a
Comment 7 Nick Clemens 2021-05-12 12:32:27 UTC
This does seem to decrease memory, but I found it tended to run a bit longer time wise

The code is simple, I don't like the 'while(1)'

Could the fetch of the next row be moved into the while?
Comment 8 Ere Maijala 2021-05-12 12:34:58 UTC
Nick, it's the same setup as in Biblio's iterator. If that's to be changed, I'd rather do it separately.
Comment 9 Nick Clemens 2021-05-12 12:54:54 UTC
Created attachment 120866 [details] [review]
Bug 28268: Improve memory usage when indexing authorities in Elasticsearch

Retrieves the complete records one by one to avoid huge memory usage.

Note that this removes the call to GuessAuthTypeCode, but it is done later in Koha::SearchEngine::Elasticsearch::marc_records_to_documents (and was never done if you asked to index a single record with --authid parameter).

Test plan:
1. Apply patch
2. Reindex authorities: perl misc/search_tools/rebuild_elasticsearch.pl -a -d -v
3. Check that indexing completed successfully and results are correct.

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Jonathan Druart 2021-05-17 14:20:00 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 11 Fridolin Somers 2021-05-18 10:18:45 UTC
Pushed to 20.11.x for 20.11.06
Comment 12 Andrew Fuerste-Henry 2021-05-25 13:45:11 UTC
Doesn't apply cleanly to 20.05, please rebase if needed