Bugzilla – Attachment 107855 Details for
Bug 25265
Elasticsearch - Batch editing items on a biblio can lead to incorrect index
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 25265: Prevent double reindex of the same item in batchmod
Bug-25265-Prevent-double-reindex-of-the-same-item-.patch (text/plain), 10.90 KB, created by
Nick Clemens (kidclamp)
on 2020-08-05 17:59:39 UTC
(
hide
)
Description:
Bug 25265: Prevent double reindex of the same item in batchmod
Filename:
MIME Type:
Creator:
Nick Clemens (kidclamp)
Created:
2020-08-05 17:59:39 UTC
Size:
10.90 KB
patch
obsolete
>From 6235b31d998540a505d79f33731f560524ee3825 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Wed, 5 Aug 2020 14:20:47 +0200 >Subject: [PATCH] Bug 25265: Prevent double reindex of the same item in > batchmod > >When batch editing, 2 reindex calls are sent to ES/Zebra. >We can easily avoid that reusing the skip_modzebra_update > >Additionally we should only send one request for biblio, and we should >only do it if we succeed > >As the hwole batch mod is in a transaction it is possible to fail in which case >Zebra queue is reset, but ES indexes have already been set > >In addition to the skip param this patchset makes ModZebra take lists of biblionumbers >or records so that multiple records can be passed in one call > >Test plan: >1 - Have a bib with several items, none marked 'not for loan' >2 - Do a staff search that returns this biblio >3 - Items show as available >4 - Click on title to go to details page >5 - Edit->Item in a batch >6 - Set the not for loan status for all items >7 - Repeat your search >8 - Items show as not for loan >--- > C4/Biblio.pm | 56 ++++++++++++++++++++++++++++++------------------------- > C4/Circulation.pm | 18 ++++++++++++++++-- > C4/Items.pm | 21 +++++++++++++++++---- > tools/batchMod.pl | 19 +++++++++++++++---- > 4 files changed, 79 insertions(+), 35 deletions(-) > >diff --git a/C4/Biblio.pm b/C4/Biblio.pm >index f62798db08..8527fd5f71 100644 >--- a/C4/Biblio.pm >+++ b/C4/Biblio.pm >@@ -2539,21 +2539,23 @@ sub CountItemsIssued { > > ModZebra( $biblionumber, $op, $server, $record ); > >-$biblionumber is the biblionumber we want to index >+$biblionumbers are an arrayref of the biblionumbers we want to index, if single passed we convert to arrayref > > $op is specialUpdate or recordDelete, and is used to know what we want to do > > $server is the server that we want to update > >-$record is the update MARC record if it's available. If it's not supplied >-and is needed, it'll be loaded from the database. >+$records are an arrayref of the updated MARC records if they are available. If they are not supplied >+and are needed, they will be loaded from the database using $biblionumbers param. If a single record passed we convert to array ref > > =cut > > sub ModZebra { > ###Accepts a $server variable thus we can use it for biblios authorities or other zebra dbs >- my ( $biblionumber, $op, $server, $record ) = @_; >- $debug && warn "ModZebra: update requested for: $biblionumber $op $server\n"; >+ my ( $biblionumbers, $op, $server, $records ) = @_; >+ $biblionumbers = [$biblionumbers] if ref $biblionumbers ne 'ARRAY' && defined $biblionumbers; >+ $records = [$records] if ref $biblionumbers ne 'ARRAY' && defined $records; >+ $debug && warn "ModZebra: updates requested for: @$biblionumbers $op $server\n"; > if ( C4::Context->preference('SearchEngine') eq 'Elasticsearch' ) { > > # TODO abstract to a standard API that'll work for whatever >@@ -2566,16 +2568,18 @@ sub ModZebra { > } > ); > if ( $op eq 'specialUpdate' ) { >- unless ($record) { >- $record = GetMarcBiblio({ >- biblionumber => $biblionumber, >- embed_items => 1 }); >+ unless ($records) { >+ foreach my $biblionumber ( @$biblionumbers ){ >+ my $record = GetMarcBiblio({ >+ biblionumber => $biblionumber, >+ embed_items => 1 }); >+ push @$records, $record; >+ } > } >- my $records = [$record]; >- $indexer->update_index_background( [$biblionumber], [$record] ); >+ $indexer->update_index_background( $biblionumbers, $records ); > } > elsif ( $op eq 'recordDelete' ) { >- $indexer->delete_index_background( [$biblionumber] ); >+ $indexer->delete_index_background( $biblionumbers ); > } > else { > croak "ModZebra called with unknown operation: $op"; >@@ -2588,19 +2592,21 @@ sub ModZebra { > # at the same time > # replaced by a zebraqueue table, that is filled with ModZebra to run. > # the table is emptied by rebuild_zebra.pl script (using the -z switch) >- my $check_sql = "SELECT COUNT(*) FROM zebraqueue >- WHERE server = ? >- AND biblio_auth_number = ? >- AND operation = ? >- AND done = 0"; >- my $check_sth = $dbh->prepare_cached($check_sql); >- $check_sth->execute( $server, $biblionumber, $op ); >- my ($count) = $check_sth->fetchrow_array; >- $check_sth->finish(); >- if ( $count == 0 ) { >- my $sth = $dbh->prepare("INSERT INTO zebraqueue (biblio_auth_number,server,operation) VALUES(?,?,?)"); >- $sth->execute( $biblionumber, $server, $op ); >- $sth->finish; >+ foreach my $biblionumber ( @$biblionumbers ){ >+ my $check_sql = "SELECT COUNT(*) FROM zebraqueue >+ WHERE server = ? >+ AND biblio_auth_number = ? >+ AND operation = ? >+ AND done = 0"; >+ my $check_sth = $dbh->prepare_cached($check_sql); >+ $check_sth->execute( $server, $biblionumber, $op ); >+ my ($count) = $check_sth->fetchrow_array; >+ $check_sth->finish(); >+ if ( $count == 0 ) { >+ my $sth = $dbh->prepare("INSERT INTO zebraqueue (biblio_auth_number,server,operation) VALUES(?,?,?)"); >+ $sth->execute( $biblionumber, $server, $op ); >+ $sth->finish; >+ } > } > } > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index a9cf02ca3d..26c3ee6dd2 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -3842,9 +3842,23 @@ sub ReturnLostItem{ > MarkIssueReturned( $borrowernumber, $itemnum ); > } > >+=head2 LostItem >+ >+ LostItem( $itemnumber, $mark_lost_from, $force_mark_returned, [$params] ); >+ >+The final optional parameter, C<$params>, expected to contain >+'skip_modzebra_update' key, which relayed down to Koha::Item/store, >+there it prevents calling of ModZebra (and Elasticsearch update), >+which takes most of the time in batch adds/deletes: ModZebra better >+to be called later in C<additem.pl> after the whole loop. >+ >+$params: >+ skip_modzebra_update => 1|0 >+ >+=cut > > sub LostItem{ >- my ($itemnumber, $mark_lost_from, $force_mark_returned) = @_; >+ my ($itemnumber, $mark_lost_from, $force_mark_returned, $params) = @_; > > unless ( $mark_lost_from ) { > # Temporary check to avoid regressions >@@ -3895,7 +3909,7 @@ sub LostItem{ > > #When item is marked lost automatically cancel its outstanding transfers and set items holdingbranch to the transfer source branch (frombranch) > if (my ( $datesent,$frombranch,$tobranch ) = GetTransfers($itemnumber)) { >- Koha::Items->find($itemnumber)->holdingbranch($frombranch)->store; >+ Koha::Items->find($itemnumber)->holdingbranch($frombranch)->store({ skip_modzebra_update => $params->{skip_modzebra_update} }); > } > my $transferdeleted = DeleteTransfer($itemnumber); > } >diff --git a/C4/Items.pm b/C4/Items.pm >index f2db7a4894..29be91b952 100644 >--- a/C4/Items.pm >+++ b/C4/Items.pm >@@ -281,10 +281,23 @@ sub AddItemBatchFromMarc { > return (\@itemnumbers, \@errors); > } > >+=head2 ModItemFromMarc >+ >+my $item = ModItemFromMarc($item_marc, $biblionumber, $itemnumber[, $params]); >+ >+The final optional parameter, C<$params>, expected to contain >+'skip_modzebra_update' key, which relayed down to Koha::Item/store, >+there it prevents calling of ModZebra (and Elasticsearch update), >+which takes most of the time in batch adds/deletes: ModZebra better >+to be called later in C<additem.pl> after the whole loop. >+ >+$params: >+ skip_modzebra_update => 1|0 >+ >+=cut >+ > sub ModItemFromMarc { >- my $item_marc = shift; >- my $biblionumber = shift; >- my $itemnumber = shift; >+ my ( $item_marc, $biblionumber, $itemnumber, $params ) = @_; > > my $frameworkcode = C4::Biblio::GetFrameworkCode($biblionumber); > my ( $itemtag, $itemsubfield ) = C4::Biblio::GetMarcFromKohaField( "items.itemnumber" ); >@@ -313,7 +326,7 @@ sub ModItemFromMarc { > $item_object = $item_object->set_or_blank($item); > my $unlinked_item_subfields = _get_unlinked_item_subfields( $localitemmarc, $frameworkcode ); > $item_object->more_subfields_xml(_get_unlinked_subfields_xml($unlinked_item_subfields)); >- $item_object->store; >+ $item_object->store({ skip_modzebra_update => $params->{skip_modzebra_update} }); > > return $item_object->unblessed; > } >diff --git a/tools/batchMod.pl b/tools/batchMod.pl >index fe40f84c7e..9e20ba4ac9 100755 >--- a/tools/batchMod.pl >+++ b/tools/batchMod.pl >@@ -184,6 +184,7 @@ if ($op eq "action") { > } > } > >+ my $biblionumbers; > try { > my $schema = Koha::Database->new->schema; > $schema->txn_do( >@@ -201,6 +202,7 @@ if ($op eq "action") { > my $return = $item->safe_delete; > if ( ref( $return ) ) { > $deleted_items++; >+ push @$biblionumbers, $itemdata->{'biblionumber'}; > } > else { > $not_deleted_items++; >@@ -280,15 +282,21 @@ if ($op eq "action") { > my $item = ModItemFromMarc( > $localmarcitem, > $itemdata->{biblionumber}, >- $itemnumber >+ $itemnumber, >+ { skip_modzebra_update => 1 }, > ) > ) > { >- LostItem( $itemnumber, 'batchmod' ) >- if $item->{itemlost} >+ LostItem( >+ $itemnumber, >+ 'batchmod', >+ undef, >+ { skip_modzebra_update => 1 } >+ ) if $item->{itemlost} > and not $itemdata->{itemlost}; > } > }; >+ push @$biblionumbers, $itemdata->{'biblionumber'}; > } > if ($runinbackground) { > $modified_items++ if $modified; >@@ -317,7 +325,10 @@ if ($op eq "action") { > } > die "Something terrible has happened!" > if ($_ =~ /Rollback failed/); # Rollback failed >- } >+ }; >+ $biblionumbers = [ uniq @$biblionumbers ]; >+ my $op = $del ? "delete" : "specialUpdate"; >+ ModZebra( $biblionumbers, $op, 'biblioserver', undef ); > } > } > # >-- >2.11.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 25265
:
107832
|
107855
|
107867
|
109538
|
109556
|
110262
|
110263
|
110264
|
110380
|
110381
|
110382
|
110457
|
110458
|
110459
|
110560
|
110561
|
110562
|
110565
|
110594
|
110595
|
110596
|
110597
|
110615
|
110618
|
110729
|
110730
|
110731
|
110732
|
110733
|
110734
|
110736
|
110737
|
110882
|
110883
|
110937