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
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
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>
It doesn't break current behavior. I tested reindexing, searching and run the SearchEngine related tests. QA scripts happy.
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.
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>
It passes all my manual and automated tests. If it does cause problems, it'll be easy enough to revert.
Pushed for 25.05! Well done everyone, thank you!