Bug 20377 - Add transaction and skip_merge to 'remove_unused_authorities.pl' script
Summary: Add transaction and skip_merge to 'remove_unused_authorities.pl' script
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Mason James
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-12 23:18 UTC by Mason James
Modified: 2018-11-20 09:39 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 20377: Various speed tweaks to 'remove_unused_authorities.pl' script (3.00 KB, patch)
2018-03-12 23:24 UTC, Mason James
Details | Diff | Splinter Review
Bug 20377: Various speed tweaks to 'remove_unused_authorities.pl' script (3.03 KB, patch)
2018-03-13 13:49 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 20377: Various speed tweaks to 'remove_unused_authorities.pl' script (3.12 KB, patch)
2018-03-23 09:42 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 Mason James 2018-03-12 23:18:04 UTC
Here is a patch to make 'remove_unused_authorities.pl' a bit faster
i get a 5x speedup after this patch

fyi: i pinched most of this patch from bulkmarcimport.pl


1/ import a bunch of authority record into a Koha, and index

2/ run script, note execution time
  $ time perl ./misc/migration_tools/remove_unused_authorities.pl 
  real    0m50s

3/ apply patch

4/ run script again, and notice it's a bit faster
  $ time perl ./misc/migration_tools/remove_unused_authorities.pl 
  real    0m10s
Comment 1 Mason James 2018-03-12 23:24:20 UTC
Created attachment 72727 [details] [review]
Bug 20377: Various speed tweaks to 'remove_unused_authorities.pl' script

Here is a patch to make 'remove_unused_authorities.pl' a bit faster
i get a 5x speedup after this patch

fyi: i pinched most of this patch from bulkmarcimport.pl

1/ import a bunch of authority record into a Koha, and index

2/ run script, note execution time
  $ time perl ./misc/migration_tools/remove_unused_authorities.pl
  real    0m50s

3/ apply patch

4/ run script again, and notice it's a bit faster
  $ time perl ./misc/migration_tools/remove_unused_authorities.pl
  real    0m10s
Comment 2 Brendan Gallagher 2018-03-13 13:49:12 UTC
Created attachment 72775 [details] [review]
Bug 20377: Various speed tweaks to 'remove_unused_authorities.pl' script

Here is a patch to make 'remove_unused_authorities.pl' a bit faster
i get a 5x speedup after this patch

fyi: i pinched most of this patch from bulkmarcimport.pl

1/ import a bunch of authority record into a Koha, and index

2/ run script, note execution time
  $ time perl ./misc/migration_tools/remove_unused_authorities.pl
  real    0m50s

3/ apply patch

4/ run script again, and notice it's a bit faster
  $ time perl ./misc/migration_tools/remove_unused_authorities.pl
  real    0m10s

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 3 Marcel de Rooy 2018-03-23 09:18:40 UTC
Note: My first impression about your use of $commit was that we should not use it that way (I would interpret it as a confirm flag like in other cron jobs), but bulkmarcimport is using it like you do here..
Comment 4 Marcel de Rooy 2018-03-23 09:42:04 UTC
Created attachment 73166 [details] [review]
Bug 20377: Various speed tweaks to 'remove_unused_authorities.pl' script

Here is a patch to make 'remove_unused_authorities.pl' a bit faster
i get a 5x speedup after this patch

fyi: i pinched most of this patch from bulkmarcimport.pl

1/ import a bunch of authority record into a Koha, and index

2/ run script, note execution time
  $ time perl ./misc/migration_tools/remove_unused_authorities.pl
  real    0m50s

3/ apply patch

4/ run script again, and notice it's a bit faster
  $ time perl ./misc/migration_tools/remove_unused_authorities.pl
  real    0m10s

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2018-03-23 09:43:14 UTC
Combining -test and -commit is kind of confusing but could be considered outside the scope? Would welcome a confirm flag instead of having to add -test.
Comment 6 Jonathan Druart 2018-03-26 18:26:36 UTC
- Please improve bug title and commit messages (there are no various tweaks, just using a transaction)
- specify default value in POD
- maybe we should remove 'c' to avoid the ambiguity with confirm?
- transaction must be done using $schema->txn_do
Comment 7 Jonathan Druart 2018-03-26 18:27:47 UTC
+        DelAuthority({ authid => $data->{'authid'}, skip_merge => 1 }) unless $test;

Checking $test must be done on the commit statements