[Bug 42879] New: Avoid per-record plugin lookup in ES marc_records_to_documents
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42879 Bug ID: 42879 Summary: Avoid per-record plugin lookup in ES marc_records_to_documents Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Searching - Elasticsearch Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@openfifth.co.uk QA Contact: testopia@bugs.koha-community.org Target Milestone: --- marc_records_to_documents invokes Koha::Plugins->call('elasticsearch_to_document', ...) once per record. Each call discovers the plugins implementing the hook, which runs a query against plugin_data -- even when no plugin implements the hook or plugins are disabled. In a batch reindex this is one extra query per record purely to discover there is nothing to call. The set of implementing plugins is constant for the batch. Resolve it once (or short-circuit when plugins are disabled / none implement the hook), then dispatch per record. Test plan: 1. prove t/db_dependent/Koha/SearchEngine/Elasticsearch.t 2. With a plugin implementing elasticsearch_to_document, confirm the hook still fires for every record. 3. Count DB statements during a batch reindex before/after. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42879 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42881 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42881 [Bug 42881] Improve Elasticsearch Indexing performance -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42879 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42879 --- Comment #1 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 200642 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200642&action=edit Bug 42879: Avoid per-record plugin lookup in ES marc_records_to_documents Before this patch, Koha::Plugins->call('elasticsearch_to_document', ...) was invoked once per record inside the foreach loop in marc_records_to_documents(). Each call internally invokes GetPlugins() to discover which plugins implement the hook, which queries the plugin_data table. This discovery query runs N times for N records, even when zero plugins implement the hook or plugins are disabled. After this patch, the elasticsearch_to_document plugins are resolved once per batch (before the per-record loop) using Koha::Plugins->new()->GetPlugins() with a method filter. The result is cached in a boolean flag. The per-record Koha::Plugins->call() is now guarded by that flag, so: 1. When plugins are disabled (Koha::Plugins->new returns undef), no lookup occurs. 2. When no plugins implement the hook, GetPlugins() is called once (not N times). 3. When plugins exist, the same call path is used, preserving all existing behavior. Variant implemented: SAFE minimal approach - Compute a single boolean flag $has_elasticsearch_plugins before the loop - Only invoke Koha::Plugins->call() when the flag is true - This avoids per-record plugin discovery for the common (no-plugin) case - The proven call path is preserved when plugins are present Test plan: 1. Verify marc_records_to_documents produces correct document output (existing tests) 2. Run prove t/db_dependent/Koha/SearchEngine/Elasticsearch.t to ensure all subtests pass 3. Verify with KTD: index a batch of records and confirm correct Elasticsearch documents 4. Optionally: enable a test plugin with elasticsearch_to_document hook and verify it is still invoked -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42879 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |martin.renvoize@openfifth.c |ity.org |o.uk -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42879 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42879 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |nick@bywatersolutions.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42879 --- Comment #2 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 208523 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208523&action=edit Benchmark scripts for full-reindex / batch indexing (bug 42881 family) Clarifying the goal of this work (bug 42881 family): the target is infrastructure-facing - the time and CPU a FULL REINDEX (misc/search_tools/rebuild_elasticsearch.pl) and LARGE BATCH re-indexes (batch record modification, imports; these enqueue index jobs of up to chunk_size = 5000 records) consume. It is not about how quickly a cataloguer sees one edit: in KTD we measured the edit-to-searchable time through the stock es_indexer_daemon and it is dominated by job dispatch (roughly one job per second), so these patches do not change it. I am attaching a small benchmark package (bug42881-es-indexing-bench.tar.gz, see README.txt inside) so testers can reproduce the before/after numbers below and, importantly, verify that the generated Elasticsearch documents are byte-for-byte identical (SHA-256 over every document) with and without the patch. Setup, once, in a KTD with Elasticsearch (stock sample data works; 10,000 generated records give steadier numbers - gen_records.pl + bulkmarcimport.pl, see README): mkdir -p _bench && cp <extracted files> _bench/ Test plan for this bug 1. prove t/db_dependent/Koha/SearchEngine/Elasticsearch.t 2. Before/after, with this patch applied on a branch: _bench/ab_bench.sh main HEAD Expect: "output identical to baseline" = yes; the plugin_methods lookup happens once per batch instead of once per record (DB statements per record fall by 1 at batch 500 and 10; at batch 1 there is still one lookup per call). 3. Plugins still work: install a plugin implementing the elasticsearch_to_document hook (adding a field to the document), reindex a record, and confirm the field is present in the ES document. Then disable the plugin (or plugins entirely) and confirm indexing still works and the field is gone. 4. Optional, full reindex wall-clock: bash _bench/bench_rebuild.sh before / after. Our results (10,436 generated records, KTD + ES 9, one core, best of 3 interleaved rounds, spread 0-4%), decode + mapping, ms per record: batch 1: 24.8 -> 23.6 (1.05x) batch 10: 6.6 -> 5.0 (1.34x) batch 500: 4.6 -> 2.9 (1.59x) full rebuild_elasticsearch.pl: 103.8 s -> 86.1 s (1.21x) As with 42878 this removes a per-record database round trip, so we expect a larger effect against a networked database. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42879 --- Comment #3 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 208528 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208528&action=edit Bug 42879: Avoid per-record plugin lookup in ES marc_records_to_documents (rebased) Rebased on top of 42878, which this bug now depends on. The module change is unchanged (only surrounding context differs); the new unit test was re-placed and the Test::More plan count updated so the series applies in sequence. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42879 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #200642|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42879 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |42878 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42878 [Bug 42878] Batch the available-item check in ES marc_records_to_documents -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42879 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42880 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42880 [Bug 42880] Avoid the USMARC round-trip when building ES documents -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon@bugs.koha-community.org