Bug 35466

Summary: bulkmarcimport needs a parameter to skip indexing
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: Command-line UtilitiesAssignee: Nick Clemens (kidclamp) <nick>
Status: Pushed to oldstable --- QA Contact: Thomas Klausner <domm>
Severity: normal    
Priority: P5 - low CC: domm, fridolin.somers, jonathan.druart, lucas, michaela.sieber, nick, phil, robin
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37478
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This patch adds a new option to skip indexing to bulkmarcimport: `--skip_indexing` It also fixes a bug where authorities were being indexed multiple times during import.
Version(s) released in:
24.11.00,24.05.06
Circulation function:
Attachments: Bug 35466: Add skip_indexing parameter to bulkmarciport.pl
Test auth file
Test biblios file
Bug 35466: (follow-up) Go to next if error, add to indexing call if not
Bug 35466: Add skip_indexing parameter to bulkmarciport.pl
Bug 35466: (follow-up) Go to next if error, add to indexing call if not
Bug 35466: (follow-up) document skip_indexing parameter to bulkmarciport.pl
Bug 35466: Add skip_indexing parameter to bulkmarciport.pl
Bug 35466: (follow-up) Go to next if error, add to indexing call if not
Bug 35466: (follow-up) document skip_indexing parameter to bulkmarciport.pl

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
Comment 19 Fridolin Somers 2024-12-13 14:32:02 UTC
Does not apply easy on 23.11.x
Please provided a specific patch if you can