https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42835
Martin Renvoize (ashimema) <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #36 from Martin Renvoize (ashimema) <[email protected]> --- Some things coming out of a QA review: 1. misc/search_tools/rebuild_elasticsearch.pl:269-271, _do_reindex_items — the new --items path isn't slice-aware. Biblios/authorities reindexing forks --processes N workers and gives each a slice => {index, count} so the table is partitioned across them. _do_reindex_items ignores %iterator_options entirely and does Koha::Items->search({}) unconditionally — every forked worker (not just the parent) reindexes the entire items table independently. Still correct (idempotent by itemnumber), but on a large catalog --items --processes 4 does 4x the ES writes and DB scans instead of dividing the work, defeating the point of --processes. 2. Koha/Item.pm:73-78 (%ITEM_CIRC_FIELDS) — holdingbranch is deliberately in the "skip biblio reindex" set (per the commit message: "AddIssue always sets it alongside onloan"), so ordinary checkouts still qualify for the fast path. But that means a genuine branch transfer (also just a holdingbranch change) skips the biblio reindex too. The items-index facets stay correct (they're queried live), but the ES-stored MARC blob used to render search-result rows (branch/location shown per hit) goes stale until some unrelated reindex trigger fires. Net effect: after a transfer, the facet count and the actual result-row "held at" branch can disagree. 3. No test coverage for the actual N+1 fix — C4/Search.pm's new pre-scan/batched visibility check (searchResults() ~1740-1765, ~1895-1905) has no corresponding change in t/db_dependent/Search.t. Every other piece of this patchset (readiness gate, caching, items index CRUD) has solid test coverage confirmed by direct reading; this is the one production change that ships untested — worth a test with a mixed visible/hidden batch, an empty batch, and the fallback-to-per-item path. And, some possible nice to haves: 1. C4/Search.pm:1750/1752 vs 1774/1778 — the visibility pre-scan parses every page's MARC record with MARC::Record->new_from_usmarc/new_record_from_zebra, then the main loop parses the same record again. Cheap relative to the DB round-trips saved, but stashing the already-parsed record from the pre-scan and reusing it would remove the duplicate work entirely. 2. Koha/SearchEngine/Elasticsearch.pm — adds Readonly our $ITEMS_INDEX => 'items'; locally even though the file now also does use Koha::SearchEngine;, which defines the same constant. Matches the file's pre-existing (already duplicated) $BIBLIOS_INDEX/$AUTHORITIES_INDEX pattern, so not new, but a third duplicate constant is a good excuse to collapse to one source of truth. 3. POD is thin on Koha::Item::_update_es_index and Koha::SearchEngine::Elasticsearch::Indexer::_item_to_document (headers present, no real description). 4. Search.pm:search_compat now always adds a _biblionumbers terms aggregation (size => 9_000) plus a second ES round-trip to the items index for facets, on every biblios-index search once the items index is ready — even for callers that don't use the returned facets. Not a bug, but worth confirming it doesn't add measurable per-search latency on large result sets. -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list -- [email protected] To unsubscribe send an email to [email protected] website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
