Bug 39076 - Elasticsearch timeouts when committing import batches
Summary: Elasticsearch timeouts when committing import batches
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Nick Clemens (kidclamp)
QA Contact: David Cook
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-02-10 13:09 UTC by Nick Clemens (kidclamp)
Modified: 2025-02-13 15:20 UTC (History)
2 users (show)

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


Attachments
Bug 39076: Use state variable to avoid creating extra ES connections (1.60 KB, patch)
2025-02-10 13:23 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 39076: Use state variable to avoid creating extra ES connections (1.66 KB, patch)
2025-02-10 14:05 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 39076: Use state variable to avoid creating extra ES connections (1.71 KB, patch)
2025-02-10 23:40 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2025-02-10 13:09:26 UTC
We have seen in production that the ES server can get overwhelmed and start issuing timeouts during imports.

The timeout errors often come from authorities linking during the bib import process
Comment 1 Nick Clemens (kidclamp) 2025-02-10 13:23:22 UTC
Created attachment 177692 [details] [review]
Bug 39076: Use state variable to avoid creating extra ES connections

As elsewhere in the code, we should use a state variable for our
'bib_searcher' object to avoid overhead of extra ES connections.

Worth noting - we don't actually use the bib_search at all during
record auto linking as we pass skipmetadata

To test:
0 - Apply patch
1 - Enable biblio auto linking
    AutoLinkBiblios, CatalogModuleRelink, LinkerRelink
2 - Stage and import a marc file
3 - Confirm record authorities link as expected
4 - Search authorities in the staff interface
5 - Confirm usage counts are shown for results
Comment 2 Tomás Cohen Arazi (tcohen) 2025-02-10 14:05:48 UTC
Created attachment 177695 [details] [review]
Bug 39076: Use state variable to avoid creating extra ES connections

As elsewhere in the code, we should use a state variable for our
'bib_searcher' object to avoid overhead of extra ES connections.

Worth noting - we don't actually use the 'bib_searcher' at all during
record auto linking as we pass skipmetadata

To test:
0 - Apply patch
1 - Enable biblio auto linking
    AutoLinkBiblios, CatalogModuleRelink, LinkerRelink
2 - Stage and import a marc file
3 - Confirm record authorities link as expected
4 - Search authorities in the staff interface
5 - Confirm usage counts are shown for results

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi (tcohen) 2025-02-10 14:08:50 UTC
It doesn't break current behavior. I tested reindexing, searching and run the SearchEngine related tests.
QA scripts happy.
Comment 4 David Cook 2025-02-10 23:10:43 UTC
So I was tempted to say this isn't really a Koha-ish way of doing things... but it turns out that it is! 

It's already done heaps in C4/Heading.pm and C4/Matcher.pm

Normally, I think that we'd just store the Koha::SearchEngine::Elasticsearch::Search object in $self (as an attribute of Koha::SearchEngine::Search that is), but that would take more code. It might be easier to understand for someone unfamiliar with "state", but it's less elegant for sure.

While "state" seems like it could be abused in some scenarios, I think that this one is a pretty reasonable place to use it.
Comment 5 David Cook 2025-02-10 23:40:00 UTC
Created attachment 177714 [details] [review]
Bug 39076: Use state variable to avoid creating extra ES connections

As elsewhere in the code, we should use a state variable for our
'bib_searcher' object to avoid overhead of extra ES connections.

Worth noting - we don't actually use the 'bib_searcher' at all during
record auto linking as we pass skipmetadata

To test:
0 - Apply patch
1 - Enable biblio auto linking
    AutoLinkBiblios, CatalogModuleRelink, LinkerRelink
2 - Stage and import a marc file
3 - Confirm record authorities link as expected
4 - Search authorities in the staff interface
5 - Confirm usage counts are shown for results

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 6 David Cook 2025-02-10 23:40:49 UTC
It passes all my manual and automated tests.

If it does cause problems, it'll be easy enough to revert.
Comment 7 Katrin Fischer 2025-02-13 15:20:46 UTC
Pushed for 25.05!

Well done everyone, thank you!