From: Bill Erickson Date: Wed, 22 Mar 2017 15:47:21 +0000 (-0400) Subject: LP#1677661 Hold Targeter V2 Repairs & Improvements X-Git-Url: https://old-git.evergreen-ils.org/?a=commitdiff_plain;h=448057dd9eaeea988cb6bdaa495f0541c317344d;p=contrib%2FConifer.git LP#1677661 Hold Targeter V2 Repairs & Improvements * Make the batch targeter more resilient to a single-hold failure. * Additional batch targeter info logging. * Set OSRF_LOG_CLIENT in hold_targeter_v2.pl for log tracing * Removes the confusingly name --target-all option * Adds a new --next-check-interval option for specifying when the targeter will next affect the currently processed holds, which may be different that now + retarget-interval in cases where the targeter is not constantly running. * Replaces the --skip-viable option with a new --soft-retarget-interval option, allowing for time-based soft-targeting. * Soft-targeting now updates hold_copy_maps for all affected holds, not just those requiring a full retarget. Signed-off-by: Bill Erickson Signed-off-by: Jason Stephenson --- diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/HoldTargeter.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/HoldTargeter.pm index 037f2307bb..22fd3e9e51 100644 --- a/Open-ILS/src/perlmods/lib/OpenILS/Application/HoldTargeter.pm +++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/HoldTargeter.pm @@ -4,6 +4,7 @@ use warnings; use OpenILS::Application; use base qw/OpenILS::Application/; use OpenILS::Utils::HoldTargeter; +use OpenSRF::Utils::Logger qw(:logger); __PACKAGE__->register_method( method => 'hold_targeter', @@ -50,15 +51,26 @@ sub hold_targeter { my $throttle = $args->{return_throttle} || 1; my $count = 0; - for my $hold_id ($targeter->find_holds_to_target) { + my @hold_ids = $targeter->find_holds_to_target; + my $total = scalar(@hold_ids); + + $logger->info("targeter processing $total holds"); + + for my $hold_id (@hold_ids) { $count++; - my $single = OpenILS::Utils::HoldTargeter::Single->new( - parent => $targeter, - skip_viable => $args->{skip_viable} - ); + my $single = + OpenILS::Utils::HoldTargeter::Single->new(parent => $targeter); - $single->target($hold_id); + # Don't let an explosion on a single hold stop processing + eval { $single->target($hold_id) }; + + if ($@) { + my $msg = "Targeter failed processing hold: $hold_id : $@"; + $single->error(1); + $logger->error($msg); + $single->message($msg) unless $single->message; + } if (($count % $throttle) == 0) { # Time to reply to the caller. Return either the number @@ -66,6 +78,8 @@ sub hold_targeter { my $res = $args->{return_count} ? $count : $single->result; $client->respond($res); + + $logger->info("targeted $count of $total holds"); } } diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Utils/HoldTargeter.pm b/Open-ILS/src/perlmods/lib/OpenILS/Utils/HoldTargeter.pm index eaed9b411b..b8553d3bec 100644 --- a/Open-ILS/src/perlmods/lib/OpenILS/Utils/HoldTargeter.pm +++ b/Open-ILS/src/perlmods/lib/OpenILS/Utils/HoldTargeter.pm @@ -58,19 +58,19 @@ sub new { # retarget_interval => # Override the 'circ.holds.retarget_interval' global_flag value. # -# newest_first => 1 -# Target holds in reverse order of create_time. +# soft_retarget_interval => +# Apply soft retarget logic to holds whose prev_check_time sits +# between the retarget_interval and the soft_retarget_interval. # -# skip_viable => 1 -# Avoid retargeting holds whose current_copy is still viable and -# permitted. This is useful for repairing holds whose targeted copy -# has become non-viable for a given hold because its status changed or -# policies affecting the hold/copy no longer allow it to be targeted. -# This setting can be used in conjunction with any other settings. +# next_check_interval => +# Use this interval to determine when the targeter will run next +# instead of relying on the retarget_interval. This value is used +# to determine if an org unit will be closed during the next iteration +# of the targeter. Applying a specific interval is useful when +# the retarget_interval is shorter than the time between targeter runs. # -# target_all => 1 -# USE WITH CAUTION. Forces (re)targeting of all active holds. This -# is primarily useful or testing. +# newest_first => 1 +# Target holds in reverse order of create_time. # # parallel_count => n # Number of parallel targeters running. This acts as the indication @@ -91,10 +91,9 @@ sub target { my @responses; for my $hold_id ($self->find_holds_to_target) { - my $single = OpenILS::Utils::HoldTargeter::Single->new( - parent => $self, - skip_viable => $args{skip_viable} - ); + my $single = + OpenILS::Utils::HoldTargeter::Single->new(parent => $self); + $single->target($hold_id); push(@responses, $single->result) unless $self->{return_count}; $count++; @@ -128,19 +127,14 @@ sub find_holds_to_target { ] }; - if (!$self->{target_all}) { - # Unless we're retargeting all holds, limit to holds that have no - # prev_check_time or those whose prev_check_time occurred - # before the retarget interval. - - my $date = DateTime->now->subtract( - seconds => $self->{retarget_interval}); - - $query->{where}->{'-or'} = [ - {prev_check_time => undef}, - {prev_check_time => {'<=' => $date->strftime('%F %T%z')}} - ]; - } + # Target holds that have no prev_check_time or those whose re-target + # time has come. If a soft_retarget_time is specified, that acts as + # the boundary. Otherwise, the retarget_time is used. + my $start_time = $self->{soft_retarget_time} || $self->{retarget_time}; + $query->{where}->{'-or'} = [ + {prev_check_time => undef}, + {prev_check_time => {'<=' => $start_time->strftime('%F %T%z')}} + ]; # parallel < 1 means no parallel my $parallel = ($self->{parallel_count} || 0) > 1 ? @@ -206,55 +200,76 @@ sub init { my $self = shift; my $e = $self->editor; - my $closed_orgs_query = { - close_start => {'<=', 'now'}, - close_end => {'>=', 'now'} - }; + # See if the caller provided an interval + my $interval = $self->{retarget_interval}; - if (!$self->{target_all}) { + if (!$interval) { + # See if we have a global flag value for the interval - # See if the caller provided an interval - my $interval = $self->{retarget_interval}; + $interval = $e->search_config_global_flag({ + name => 'circ.holds.retarget_interval', + enabled => 't' + })->[0]; - if (!$interval) { - # See if we have a global flag value for the interval + # If no flag is present, default to a 24-hour retarget interval. + $interval = $interval ? $interval->value : '24h'; + } - $interval = $e->search_config_global_flag({ - name => 'circ.holds.retarget_interval', - enabled => 't' - })->[0]; + my $retarget_seconds = interval_to_seconds($interval); - # If no flag is present, default to a 24-hour retarget interval. - $interval = $interval ? $interval->value : '24h'; - } + $self->{retarget_time} = DateTime->now(time_zone => 'local') + ->subtract(seconds => $retarget_seconds); - # Convert the interval to seconds for current and future use. - $self->{retarget_interval} = interval_to_seconds($interval); + $logger->info("Using retarget time: ". + $self->{retarget_time}->strftime('%F %T%z')); - # An org unit is considered closed for retargeting purposes - # if it's closed both now and at the next re-target date. + if ($self->{soft_retarget_interval}) { - my $next_check_time = - DateTime->now->add(seconds => $self->{retarget_interval}) - ->strftime('%F %T%z'); + my $secs = OpenSRF::Utils->interval_to_seconds( + $self->{soft_retarget_interval}); - $closed_orgs_query = { - '-and' => [ - $closed_orgs_query, { - close_start => {'<=', $next_check_time}, - close_end => {'>=', $next_check_time} - } - ] - } + $self->{soft_retarget_time} = + DateTime->now(time_zone => 'local')->subtract(seconds => $secs); + + $logger->info("Using soft retarget time: " . + $self->{soft_retarget_time}->strftime('%F %T%z')); } - my $closed = - $self->editor->search_actor_org_unit_closed_date($closed_orgs_query); + # Holds targeted in the current targeter instance not be retargeted + # until the next check date. If a next_check_interval is provided + # it overrides the retarget_interval. + my $next_check_secs = + $self->{next_check_interval} ? + OpenSRF::Utils->interval_to_seconds($self->{next_check_interval}) : + $retarget_seconds; + + my $next_check_date = + DateTime->now(time_zone => 'local')->add(seconds => $next_check_secs); + + my $next_check_time = $next_check_date->strftime('%F %T%z'); + + $logger->info("Next check time: $next_check_time"); + + # An org unit is considered closed for retargeting purposes + # if it's closed both now and at the next re-target date. + my $closed = $self->editor->search_actor_org_unit_closed_date({ + '-and' => [{ + close_start => {'<=', 'now'}, + close_end => {'>=', 'now'} + }, { + close_start => {'<=', $next_check_time}, + close_end => {'>=', $next_check_time} + }] + }); + + my @closed_orgs = map {$_->org_unit} @$closed; + $logger->info("closed org unit IDs: @closed_orgs"); # Map of org id to 1. Any org in the map is closed. - $self->{closed_orgs} = {map {$_->org_unit => 1} @$closed}; + $self->{closed_orgs} = {map {$_ => 1} @closed_orgs}; } + # Org unit setting fetch+cache # $e is the OpenILS::Utils::HoldTargeter::Single editor. Use it if # provided to avoid timeouts on the in-transaction child editor. @@ -453,7 +468,8 @@ sub handle_expired_hold { my $ex_time = $dt_parser->parse_datetime(cleanse_ISO8601($hold->expire_time)); - return 1 unless DateTime->compare($ex_time, DateTime->now) < 0; + return 1 unless + DateTime->compare($ex_time, DateTime->now(time_zone => 'local')) < 0; # Hold is expired -- @@ -821,24 +837,40 @@ sub inspect_previous_target { # exit if previous target is no longer valid. return 1 unless $prev; - if ($self->{skip_viable}) { - # In skip_viable mode, leave the hold as-is if the existing - # current_copy is still permitted. - # Note: viability checking is done this late in the process - # (specifically after other potential copies have been fetched) - # because we first need to confirm the current_copy is a valid - # potential copy (e.g. it's holdable, non-deleted, etc.), which - # copy_is_permitted, which only checks hold matrix policies, - # does not check. + my $soft_retarget = 0; + + if ($self->parent->{soft_retarget_time}) { + # A hold is soft-retarget-able if its prev_check_time is + # later then the retarget_time, i.e. it sits between the + # soft_retarget_time and the retarget_time. - return $self->exit_targeter("Skipping with viable target = $prev_id") - if $self->copy_is_permitted($prev); + my $pct = $dt_parser->parse_datetime( + cleanse_ISO8601($hold->prev_check_time)); - # Previous copy is now confirmed non-viable. + $soft_retarget = + DateTime->compare($pct, $self->parent->{retarget_time}) > 0; + } + + if ($soft_retarget) { + + # In soft-retarget mode, if the existing copy is still a valid + # target for the hold, exit early. + if ($self->copy_is_permitted($prev)) { + + # Commit to persist the updated action.hold_copy_map's + $self->editor->commit; + + return $self->exit_targeter( + "Exiting early on soft-retarget with viable copy = $prev_id"); + + } else { + $self->log_hold("soft retarget failed because copy $prev_id is ". + "no longer targetable for this hold. Retargeting..."); + } } else { - # Previous copy may be targetable. Keep it around for later + # Previous copy /may/ be targetable. Keep it around for later # in case we need to confirm its viability and re-use it. $self->{valid_previous_copy} = $prev; } diff --git a/Open-ILS/src/support-scripts/hold_targeter_v2.pl b/Open-ILS/src/support-scripts/hold_targeter_v2.pl index 06d1c3a501..93eaf4d0f9 100755 --- a/Open-ILS/src/support-scripts/hold_targeter_v2.pl +++ b/Open-ILS/src/support-scripts/hold_targeter_v2.pl @@ -6,6 +6,7 @@ use OpenSRF::System; use OpenSRF::AppSession; use OpenSRF::Utils::SettingsClient; use OpenILS::Utils::Fieldmapper; +$ENV{OSRF_LOG_CLIENT} = 1; #---------------------------------------------------------------- # Batch hold (re)targeter # @@ -18,25 +19,25 @@ my $osrf_config = '/openils/conf/opensrf_core.xml'; my $lockfile = '/tmp/hold_targeter-LOCK'; my $parallel = 0; my $verbose = 0; -my $target_all; -my $skip_viable; my $retarget_interval; +my $soft_retarget_interval; +my $next_check_interval; my $recv_timeout = 3600; my $parallel_init_sleep = 0; # how often the server sends a summary reply per backend. -my $return_throttle = 50; +my $return_throttle = 500; GetOptions( - 'osrf-config=s' => \$osrf_config, - 'lockfile=s' => \$lockfile, - 'parallel=i' => \$parallel, - 'verbose' => \$verbose, - 'target-all' => \$target_all, - 'skip-viable' => \$skip_viable, - 'retarget-interval=s' => \$retarget_interval, + 'help' => \$help, + 'osrf-config=s' => \$osrf_config, + 'lockfile=s' => \$lockfile, + 'parallel=i' => \$parallel, + 'verbose' => \$verbose, 'parallel-init-sleep=i' => \$parallel_init_sleep, - 'help' => \$help + 'retarget-interval=s' => \$retarget_interval, + 'next-check-interval=s' => \$next_check_interval, + 'soft-retarget-interval=s' => \$soft_retarget_interval, ) || die "\nSee --help for more\n"; sub help { @@ -58,7 +59,6 @@ General Options --lockfile [/tmp/hold_targeter-LOCK] Full path to lock file - --verbose Print process counts @@ -76,15 +76,31 @@ Targeting Options Defaults to no sleep. - --target-all - Target all active holds, regardless of when they were last targeted. - - --skip-viable - Avoid modifying holds that currently target viable copies. In - other words, only (re)target holds in a non-viable state. + --soft-retarget-interval + Holds whose previous check time sits between the + --soft-retarget-interval and the --retarget-interval are + treated like this: + + 1. The list of potential copies is updated for all matching holds. + 2. Holds that have a viable target are otherwise left untouched, + including their prev_check_time. + 3. Holds with no viable target are fully retargeted. + + --next-check-interval + Specify how long after the current run time the targeter will + retarget the currently affected holds. Applying a specific + interval is useful when the retarget_interval is shorter than + the time between targeter runs. + + This value is used to determine if an org unit will be closed + during the next iteration of the targeter. It overrides the + default behavior of calculating the next retarget time from the + retarget-interval. --retarget-interval - Override the 'circ.holds.retarget_interval' global_flag value. + Retarget holds whose previous check time occured before the + requested interval. + Overrides the 'circ.holds.retarget_interval' global_flag value. HELP @@ -130,9 +146,9 @@ sub run_batches { return_throttle => $return_throttle, parallel_count => $parallel, parallel_slot => $slot, - skip_viable => $skip_viable, - target_all => $target_all, - retarget_interval => $retarget_interval + retarget_interval => $retarget_interval, + next_check_interval => $next_check_interval, + soft_retarget_interval => $soft_retarget_interval } ); @@ -151,6 +167,8 @@ sub run_batches { for my $req (@reqs) { # Pull all responses off the receive queues. while (my $resp = $req->recv(0)) { + die $req->failed . "\n" if $req->failed; + $verbose && print sprintf( "Targeter [%d] processed %d holds\n", $req->{_parallel_slot},