Bug 39076

Summary: Elasticsearch timeouts when committing import batches
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: ToolsAssignee: Nick Clemens (kidclamp) <nick>
Status: Pushed to main --- QA Contact: David Cook <dcook>
Severity: normal    
Priority: P5 - low CC: dcook, mnero, tomascohen
Version: Main   
Hardware: All   
OS: All   
GIT URL: 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
Bug 39076: Use state variable to avoid creating extra ES connections
Bug 39076: Use state variable to avoid creating extra ES connections

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!
Comment 8 David Cook 2025-05-08 05:46:41 UTC
On a side note, does this fix your prod issue, Nick?