Bug 35466 - bulkmarcimport needs a parameter to skip indexing
Summary: bulkmarcimport needs a parameter to skip indexing
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Nick Clemens (kidclamp)
QA Contact: Thomas Klausner
URL:
Keywords: release-notes-needed
: 37565 (view as bug list)
Depends on:
Blocks:
 
Reported: 2023-12-01 16:19 UTC by Katrin Fischer
Modified: 2024-11-20 14:59 UTC (History)
7 users (show)

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


Attachments
Bug 35466: Add skip_indexing parameter to bulkmarciport.pl (4.86 KB, patch)
2024-08-05 12:51 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Test auth file (36.63 KB, application/marc)
2024-08-05 12:51 UTC, Nick Clemens (kidclamp)
Details
Test biblios file (121.36 KB, application/marc)
2024-08-05 12:52 UTC, Nick Clemens (kidclamp)
Details
Bug 35466: (follow-up) Go to next if error, add to indexing call if not (2.74 KB, patch)
2024-08-23 13:17 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35466: Add skip_indexing parameter to bulkmarciport.pl (4.92 KB, patch)
2024-08-23 15:28 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 35466: (follow-up) Go to next if error, add to indexing call if not (2.80 KB, patch)
2024-08-23 15:28 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 35466: (follow-up) document skip_indexing parameter to bulkmarciport.pl (1.11 KB, patch)
2024-10-19 12:39 UTC, Thomas Klausner
Details | Diff | Splinter Review
Bug 35466: Add skip_indexing parameter to bulkmarciport.pl (4.96 KB, patch)
2024-10-19 13:00 UTC, Thomas Klausner
Details | Diff | Splinter Review
Bug 35466: (follow-up) Go to next if error, add to indexing call if not (2.84 KB, patch)
2024-10-19 13:00 UTC, Thomas Klausner
Details | Diff | Splinter Review
Bug 35466: (follow-up) document skip_indexing parameter to bulkmarciport.pl (1.15 KB, patch)
2024-10-19 13:00 UTC, Thomas Klausner
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2023-12-01 16:19:08 UTC
When using bulkmarcimport for migrations it does make sense to do a full reindex after finishing the import. At the moment the import will always create entries in background_jobs for Elasticsearch and in zebra_queue for Zebra. 

We should have a switch to turn this off if indexing is not required to increase performance and avoid unnecessary load.
Comment 1 Katrin Fischer 2023-12-01 16:26:05 UTC
We need to adjust the calls to:
AddBiblio ModBiblio ModBiblioMarc AddItemBatchFromMarc

And add the skip_record_index parameter.
Comment 2 Katrin Fischer 2024-08-05 10:59:27 UTC
*** Bug 37565 has been marked as a duplicate of this bug. ***
Comment 3 Nick Clemens (kidclamp) 2024-08-05 12:51:07 UTC
Created attachment 170059 [details] [review]
Bug 35466: Add skip_indexing parameter to bulkmarciport.pl

This patch adds a new optoin to skip indexing to bulkmarcimport.
It also fixes a bug where authorities were being indexed multiple times
during import.

To test:
 1 - Apply patch
 2 - Download the sample files on this bug
 3 - Perform asearch engine search that should retrieve all records( 'a' in Zebra, '*' in ES)
 4 - Note the total (435 in KTD)
 5 - perl misc/migration_tools/bulkmarcimport.pl -b -v --file=bug_35466_b.mrc
 6 - Search again, note increaed by 100
 7 - perl misc/migration_tools/bulkmarcimport.pl -b -v --file=bug_35466_b.mrc --skip_indexing
 8 - Search again, no increase
 9 - perl misc/search_tools/rebuild_elasticsearch.pl -v
10 - Search again, increase, records were added but not initially indexed
11 - Browse to authorities and search as for biblios
12 - perl misc/migration_tools/bulkmarcimport.pl -a -v --file=bug_35466_a.mrc
13 - Search again, note increase
14 - perl misc/migration_tools/bulkmarcimport.pl -a -v --file=bug_35466_a.mrc --skip_indexing
15 - Search again, no increase
16 - perl misc/search_tools/rebuild_elasticsearch.pl -v
17 - Search again, increase, records were added but not initially indexed
18 - Sign off! Hi5!
Comment 4 Nick Clemens (kidclamp) 2024-08-05 12:51:47 UTC
Created attachment 170060 [details]
Test auth file
Comment 5 Nick Clemens (kidclamp) 2024-08-05 12:52:02 UTC
Created attachment 170061 [details]
Test biblios file
Comment 6 Phil Ringnalda 2024-08-23 00:12:54 UTC
Test plan fails for me at step 13 while using ElasticSearch, there's no increase.

Were authorities really being indexed multiple times? The way the indexing call is $indexer->update_index( \@search_engine_record_ids, \@search_engine_records ); and those params are only set for biblios, it looks to me from a quick skim of update_index() that for authorities we don't actually index if we don't index in ModAuthority/AddAuthority, because passing empty @record_ids and @records just indexes nothing.
Comment 7 Nick Clemens (kidclamp) 2024-08-23 13:17:49 UTC
Created attachment 170631 [details] [review]
Bug 35466: (follow-up) Go to next if error, add to indexing call if not

Before the previous patch we were indexing with every AddAuthority/ModAuthority call and I assumed
we were also indexing during the commity, however, it appears we were not. This patch ensures we push
the record and ids into the arrays for indexing during commit.

Additionally I add a skip to next record on error, to match biblio behaviour.
I also correct a log referring to biblios during authority importing
Comment 8 Nick Clemens (kidclamp) 2024-08-23 13:18:39 UTC
(In reply to Phil Ringnalda from comment #6)
> Test plan fails for me at step 13 while using ElasticSearch, there's no
> increase.
> 
> Were authorities really being indexed multiple times? The way the indexing
> call is $indexer->update_index( \@search_engine_record_ids,
> \@search_engine_records ); and those params are only set for biblios, it
> looks to me from a quick skim of update_index() that for authorities we
> don't actually index if we don't index in ModAuthority/AddAuthority, because
> passing empty @record_ids and @records just indexes nothing.

Ah, I assumed the code was doing the right thing as well as the wrong thing, I was wrong ;-)
Comment 9 Phil Ringnalda 2024-08-23 15:28:23 UTC
Created attachment 170641 [details] [review]
Bug 35466: Add skip_indexing parameter to bulkmarciport.pl

This patch adds a new optoin to skip indexing to bulkmarcimport.
It also fixes a bug where authorities were being indexed multiple times
during import.

To test:
 1 - Apply patch
 2 - Download the sample files on this bug
 3 - Perform asearch engine search that should retrieve all records( 'a' in Zebra, '*' in ES)
 4 - Note the total (435 in KTD)
 5 - perl misc/migration_tools/bulkmarcimport.pl -b -v --file=bug_35466_b.mrc
 6 - Search again, note increaed by 100
 7 - perl misc/migration_tools/bulkmarcimport.pl -b -v --file=bug_35466_b.mrc --skip_indexing
 8 - Search again, no increase
 9 - perl misc/search_tools/rebuild_elasticsearch.pl -v
10 - Search again, increase, records were added but not initially indexed
11 - Browse to authorities and search as for biblios
12 - perl misc/migration_tools/bulkmarcimport.pl -a -v --file=bug_35466_a.mrc
13 - Search again, note increase
14 - perl misc/migration_tools/bulkmarcimport.pl -a -v --file=bug_35466_a.mrc --skip_indexing
15 - Search again, no increase
16 - perl misc/search_tools/rebuild_elasticsearch.pl -v
17 - Search again, increase, records were added but not initially indexed
18 - Sign off! Hi5!

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 10 Phil Ringnalda 2024-08-23 15:28:25 UTC
Created attachment 170642 [details] [review]
Bug 35466: (follow-up) Go to next if error, add to indexing call if not

Before the previous patch we were indexing with every AddAuthority/ModAuthority call and I assumed
we were also indexing during the commity, however, it appears we were not. This patch ensures we push
the record and ids into the arrays for indexing during commit.

Additionally I add a skip to next record on error, to match biblio behaviour.
I also correct a log referring to biblios during authority importing

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 11 Thomas Klausner 2024-10-19 12:39:16 UTC
Created attachment 172997 [details] [review]
Bug 35466: (follow-up) document skip_indexing parameter to bulkmarciport.pl

Add documentation for the new -si, --skip_indexing parameter
Comment 12 Thomas Klausner 2024-10-19 13:00:09 UTC
Created attachment 172998 [details] [review]
Bug 35466: Add skip_indexing parameter to bulkmarciport.pl

This patch adds a new optoin to skip indexing to bulkmarcimport.
It also fixes a bug where authorities were being indexed multiple times
during import.

To test:
 1 - Apply patch
 2 - Download the sample files on this bug
 3 - Perform asearch engine search that should retrieve all records( 'a' in Zebra, '*' in ES)
 4 - Note the total (435 in KTD)
 5 - perl misc/migration_tools/bulkmarcimport.pl -b -v --file=bug_35466_b.mrc
 6 - Search again, note increaed by 100
 7 - perl misc/migration_tools/bulkmarcimport.pl -b -v --file=bug_35466_b.mrc --skip_indexing
 8 - Search again, no increase
 9 - perl misc/search_tools/rebuild_elasticsearch.pl -v
10 - Search again, increase, records were added but not initially indexed
11 - Browse to authorities and search as for biblios
12 - perl misc/migration_tools/bulkmarcimport.pl -a -v --file=bug_35466_a.mrc
13 - Search again, note increase
14 - perl misc/migration_tools/bulkmarcimport.pl -a -v --file=bug_35466_a.mrc --skip_indexing
15 - Search again, no increase
16 - perl misc/search_tools/rebuild_elasticsearch.pl -v
17 - Search again, increase, records were added but not initially indexed
18 - Sign off! Hi5!

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Thomas Klausner <domm@plix.at>
Comment 13 Thomas Klausner 2024-10-19 13:00:12 UTC
Created attachment 172999 [details] [review]
Bug 35466: (follow-up) Go to next if error, add to indexing call if not

Before the previous patch we were indexing with every AddAuthority/ModAuthority call and I assumed
we were also indexing during the commity, however, it appears we were not. This patch ensures we push
the record and ids into the arrays for indexing during commit.

Additionally I add a skip to next record on error, to match biblio behaviour.
I also correct a log referring to biblios during authority importing

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Thomas Klausner <domm@plix.at>
Comment 14 Thomas Klausner 2024-10-19 13:00:15 UTC
Created attachment 173000 [details] [review]
Bug 35466: (follow-up) document skip_indexing parameter to bulkmarciport.pl

Add documentation for the new -si, --skip_indexing parameter

Signed-off-by: Thomas Klausner <domm@plix.at>
Comment 15 Thomas Klausner 2024-10-19 13:04:24 UTC
Interestingly enough we needed exactly this feature today during a migration, so I QA'ed it and it works as advertised!
Comment 16 Katrin Fischer 2024-10-21 10:27:01 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 17 Katrin Fischer 2024-10-21 11:12:11 UTC
Fixed typo in bulkmarciport.pl in commit message on push.
Comment 18 Lucas Gass (lukeg) 2024-11-20 14:59:30 UTC
Backported to 24.05.x for upcoming 24.05.06