Attachment 140231 Details for Bug 27421 – Bug 27421: (QA follow-up) BatchCommitImportRecords needs param for skipping commits

[patch] Bug 27421: (QA follow-up) BatchCommitImportRecords needs param for skipping commits

Bug-27421-QA-follow-up-BatchCommitImportRecords-ne.patch (text/plain), 5.15 KB, created by Marcel de Rooy on 2022-09-06 12:48:57 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marcel de Rooy

Created: 2022-09-06 12:48:57 UTC

Size: 5.15 KB

patch

obsolete

>From 02e4e3f8f685be83b3cca7d4b13bb6ad5fcdf302 Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Date: Mon, 5 Sep 2022 11:57:52 +0000
>Subject: [PATCH] Bug 27421: (QA follow-up) BatchCommitImportRecords needs
> param for skipping commits
>Content-Type: text/plain; charset=utf-8
>
>When you submit a background jobs, and it fails, you do not expect
>partial results in the database.
>Note that when the Background feature would support a partially
>completed status, things might change again.
>
>Note that the >0 test was superfluous if you check for ^\d+$.
>
>Test plan:
>Run t/db_dependent/Koha/BackgroundJobs/StageMARCForImport.t
>
>Note: This serves to verify that it still runs as expected.
>The test plan of the following patch covers the new param.
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>---
> C4/ImportBatch.pm                           | 38 +++++++++------------
> Koha/BackgroundJob/MARCImportCommitBatch.pm | 13 +++----
> 2 files changed, 24 insertions(+), 27 deletions(-)
>
>diff --git a/C4/ImportBatch.pm b/C4/ImportBatch.pm
>index bd51a5c738..96ee1c45a7 100644
>--- a/C4/ImportBatch.pm
>+++ b/C4/ImportBatch.pm
>@@ -509,28 +509,23 @@ sub BatchFindDuplicates {
> 
> =head2 BatchCommitRecords
> 
>-  my ($num_added, $num_updated, $num_items_added, $num_items_replaced, $num_items_errored, $num_ignored) =
>-        BatchCommitRecords($batch_id, $framework,
>-        $progress_interval, $progress_callback);
>+    my ($num_added, $num_updated, $num_items_added, $num_items_replaced, $num_items_errored, $num_ignored) =
>+        BatchCommitRecords( $batch_id, $framework, $progress_interval, $progress_callback, $params );
>+
>+    Parameter skip_intermediate_commit does what is says.
> 
> =cut
> 
> sub BatchCommitRecords {
>-    my $batch_id = shift;
>-    my $framework = shift;
>+    my ( $batch_id, $framework, $progress_interval, $progress_callback, $params ) = @_;
>+    my $skip_intermediate_commit = $params->{skip_intermediate_commit};
>+    $progress_interval = 0 unless $progress_interval && $progress_interval =~ /^\d+$/;
>+    $progress_interval = 0 unless ref($progress_callback) eq 'CODE';
> 
>     my $schema = Koha::Database->schema;
>-
>-    # optional callback to monitor status 
>-    # of job
>-    my $progress_interval = 0;
>-    my $progress_callback = undef;
>-    if ($#_ == 1) {
>-        $progress_interval = shift;
>-        $progress_callback = shift;
>-        $progress_interval = 0 unless $progress_interval =~ /^\d+$/ and $progress_interval > 0;
>-        $progress_interval = 0 unless 'CODE' eq ref $progress_callback;
>-    }
>+    $schema->txn_begin;
>+    # NOTE: Moved this transaction to the front of the routine. Note that inside the while loop below
>+    # transactions may be committed and started too again. The final commit is close to the end.
> 
>     my $record_type;
>     my $num_added = 0;
>@@ -560,7 +555,6 @@ sub BatchCommitRecords {
> 
>     my $rec_num = 0;
>     my @biblio_ids;
>-    $schema->txn_begin; # We commit in a transaction
>     while (my $rowref = $sth->fetchrow_hashref) {
>         $record_type = $rowref->{'record_type'};
> 
>@@ -568,9 +562,9 @@ sub BatchCommitRecords {
> 
>         if ($progress_interval and (0 == ($rec_num % $progress_interval))) {
>             # report progress and commit
>-            $schema->txn_commit;
>+            $schema->txn_commit unless $skip_intermediate_commit;
>             &$progress_callback( $rec_num );
>-            $schema->txn_begin;
>+            $schema->txn_begin unless $skip_intermediate_commit;
>         }
>         if ($rowref->{'status'} eq 'error' or $rowref->{'status'} eq 'imported') {
>             $num_ignored++;
>@@ -700,8 +694,6 @@ sub BatchCommitRecords {
>         &$progress_callback($rec_num);
>     }
> 
>-    $schema->txn_commit; # Commit final records that may not have hit callback threshold
>-
>     $sth->finish();
> 
>     if ( @biblio_ids ) {
>@@ -710,6 +702,10 @@ sub BatchCommitRecords {
>     }
> 
>     SetImportBatchStatus($batch_id, 'imported');
>+
>+    # Moved final commit to the end
>+    $schema->txn_commit;
>+
>     return ($num_added, $num_updated, $num_items_added, $num_items_replaced, $num_items_errored, $num_ignored);
> }
> 
>diff --git a/Koha/BackgroundJob/MARCImportCommitBatch.pm b/Koha/BackgroundJob/MARCImportCommitBatch.pm
>index 57faa99c51..23a8123756 100644
>--- a/Koha/BackgroundJob/MARCImportCommitBatch.pm
>+++ b/Koha/BackgroundJob/MARCImportCommitBatch.pm
>@@ -67,12 +67,13 @@ sub process {
>     try {
>         my $size = Koha::Import::Records->search({ import_batch_id => $import_batch_id })->count;
>         $self->size($size)->store;
>-        (
>-            $num_added, $num_updated, $num_items_added,
>-            $num_items_replaced, $num_items_errored, $num_ignored
>-          )
>-          = BatchCommitRecords( $import_batch_id, $frameworkcode, 50,
>-            sub { my $job_progress = shift; $self->progress( $job_progress )->store } );
>+        ( $num_added, $num_updated, $num_items_added,
>+          $num_items_replaced, $num_items_errored, $num_ignored ) =
>+          BatchCommitRecords(
>+            $import_batch_id, $frameworkcode, 50,
>+            sub { my $job_progress = shift; $self->progress( $job_progress )->store },
>+            { skip_intermediate_commit => 1 },
>+        );
>     }
>     catch {
>         warn $_;
>-- 
>2.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 27421: 136127 | 136128 | 136129 | 136216 | 136217 | 136338 | 136339 | 138642 | 138643 | 138656 | 138657 | 138660 | 138881 | 139384 | 139385 | 139386 | 139387 | 139388 | 139448 | 139449 | 139450 | 139797 | 140205 | 140206 | 140207 | 140208 | 140223 | 140224 | 140225 | 140226 | 140227 | 140228 | 140229 | 140230 | 140231 | 140232 | 140233 | 140234 | 140759