Bug 38408 - Add parallel exporting of MARC records to Zebra rebuild/reindex
Summary: Add parallel exporting of MARC records to Zebra rebuild/reindex
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Marcel de Rooy
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks: 38427
  Show dependency treegraph
 
Reported: 2024-11-08 11:01 UTC by Marcel de Rooy
Modified: 2025-03-17 07:28 UTC (History)
3 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Add the parameter --processes to the rebuild_zebra.pl script for parallel processing of the export step when dumping authority or biblio records to file. This will signficantly reduce the time needed for this part of reindexing.
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 38408: Add parallel exporting in rebuild_zebra.pl (6.85 KB, patch)
2024-11-12 14:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 38408: Add parallel exporting in rebuild_zebra.pl (6.98 KB, patch)
2025-02-18 15:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 38408: Add parallel exporting in rebuild_zebra.pl (7.00 KB, patch)
2025-02-28 10:04 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 38408: Add parallel exporting in rebuild_zebra.pl (7.06 KB, patch)
2025-03-07 14:25 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38408: (QA follow-up) Add --forks to usage statement (1.15 KB, patch)
2025-03-10 10:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 38408: (QA follow-up) Rename --forks, add usage (1.44 KB, patch)
2025-03-10 10:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2024-11-08 11:01:46 UTC
We can speed up reindexing Zebra when we use parallel exporting, creating multiple export files in the same directory. This directory is passed later to zebraidx.
Comment 1 Marcel de Rooy 2024-11-12 14:41:38 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2024-11-12 14:47:22 UTC
(In reply to Marcel de Rooy from comment #1)

> NOTE: This report does not touch koha-rebuild-zebra yet! This will be
> done on a follow-up.

Planned on bug 38427
Comment 3 Marcel de Rooy 2024-11-12 15:01:41 UTC
Note for QA: A few smaller improvements are reported on bug 38427 as well.
Comment 4 David Cook 2024-11-12 23:30:16 UTC
This makes a lot of sense conceptually. I can't believe we didn't do this years ago.

I've got a few questions and comments though:

1. Is there are a reason you moved around a bunch of the top "use" statements? It makes it harder to see what's actually changed at the top.

2. There's a 'my $chunk_size = 100000;' at the top of the script which appears to be unused? 

3. It wouldn't hurt to add some more code comments to make it easier to read/understand what's happening. Not sure that I follow the math when just reading the code. I would've expected chunk_size to stay fixed and $num_records_exported to be the number of records actually exported. Not sure this all adds up... but don't have time to test right now.
Comment 5 Marcel de Rooy 2024-11-13 12:29:20 UTC
(In reply to David Cook from comment #4)

> 1. Is there are a reason you moved around a bunch of the top "use"
> statements? It makes it harder to see what's actually changed at the top.

It was kind of disorganized, and perltidy was also complaining. So I rearranged a bit more than strictly needed. But nothing special.

> 2. There's a 'my $chunk_size = 100000;' at the top of the script which
> appears to be unused? 
True. See the follow-up bug. Just a remainder of something obsoleted during dev.

> 3. It wouldn't hurt to add some more code comments to make it easier to
> read/understand what's happening. Not sure that I follow the math when just
> reading the code. I would've expected chunk_size to stay fixed and
> $num_records_exported to be the number of records actually exported. Not
> sure this all adds up... but don't have time to test right now.

I tried to put some extra comments in the main loop. The tricky thing is that the loop inside is normally the child, but could be the parent too if you dont fork.
I am using chunk_size to control the loop; so it is adjusted before the last run. Added a TODO on the follow-up bug about passing data from child back to parent.
Comment 6 Marcel de Rooy 2025-02-18 15:56:27 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2025-02-18 15:57:15 UTC
Rebased for tidy codebase stuff
Comment 8 David Cook 2025-02-19 23:37:40 UTC
Going to look at testing this one today...
Comment 9 David Cook 2025-02-21 06:09:05 UTC
(In reply to David Cook from comment #8)
> Going to look at testing this one today...

Sorry Marcel. Ran out of time this week...
Comment 10 Marcel de Rooy 2025-02-21 07:09:08 UTC
(In reply to David Cook from comment #9)
> (In reply to David Cook from comment #8)
> > Going to look at testing this one today...
> 
> Sorry Marcel. Ran out of time this week...

Too bad :) New week, new chances..
Comment 11 Paul Derscheid 2025-02-28 10:04:25 UTC Comment hidden (obsolete)
Comment 12 Martin Renvoize (ashimema) 2025-03-07 14:25:53 UTC Comment hidden (obsolete)
Comment 13 Martin Renvoize (ashimema) 2025-03-07 14:28:40 UTC
Finally found a moment to revisit this.. very nice improvement.

Passing QA
Comment 14 Marcel de Rooy 2025-03-09 11:59:58 UTC
(In reply to Martin Renvoize (ashimema) from comment #13)
> Finally found a moment to revisit this.. very nice improvement.
> 
> Passing QA

Great Martin. Thanks
Comment 15 Katrin Fischer 2025-03-10 10:37:20 UTC
Please document the new parameters in the script. 

--help gives nothing for the new parameters.

Should it be --forks?

Also release notes please!
Comment 16 Katrin Fischer 2025-03-10 10:45:27 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 17 Marcel de Rooy 2025-03-10 10:52:28 UTC
Created attachment 179096 [details] [review]
Bug 38408: (QA follow-up) Add --forks to usage statement

Test plan:
Run script with -h

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2025-03-10 10:54:44 UTC
(In reply to Katrin Fischer from comment #15)
> Please document the new parameters in the script. 
> 
> --help gives nothing for the new parameters.
> 
> Should it be --forks?
> 
> Also release notes please!

Ah we could also move to processes or so ? Just wait a bit
Comment 19 Katrin Fischer 2025-03-10 10:59:09 UTC
(In reply to Marcel de Rooy from comment #18)
> (In reply to Katrin Fischer from comment #15)
> > Please document the new parameters in the script. 
> > 
> > --help gives nothing for the new parameters.
> > 
> > Should it be --forks?
> > 
> > Also release notes please!
> 
> Ah we could also move to processes or so ? Just wait a bit

I'll wait for your "green light" before pushing the follow-ups, thanks Marcel.
Comment 20 Marcel de Rooy 2025-03-10 10:59:47 UTC
Created attachment 179097 [details] [review]
Bug 38408: (QA follow-up) Rename --forks, add usage

Test plan:
Run script with -h

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2025-03-10 11:01:19 UTC
(In reply to Katrin Fischer from comment #19)
> (In reply to Marcel de Rooy from comment #18)
> > (In reply to Katrin Fischer from comment #15)
> > > Please document the new parameters in the script. 
> > > 
> > > --help gives nothing for the new parameters.
> > > 
> > > Should it be --forks?
> > > 
> > > Also release notes please!
> > 
> > Ah we could also move to processes or so ? Just wait a bit
> 
> I'll wait for your "green light" before pushing the follow-ups, thanks
> Marcel.

Back to green
Comment 22 Katrin Fischer 2025-03-10 11:05:20 UTC
Switching status so I see it for next RM session.
Comment 23 Katrin Fischer 2025-03-17 07:28:16 UTC
Pushed for 25.05!

Well done everyone, thank you!