From: Bill Erickson Date: Wed, 22 Jun 2016 21:37:28 +0000 (-0400) Subject: LP#? Hold targeter refactoring and optimization. X-Git-Url: https://old-git.evergreen-ils.org/?a=commitdiff_plain;h=149c150e9cb13821538718195797808bb06f1640;p=working%2FEvergreen.git LP#? Hold targeter refactoring and optimization. Signed-off-by: Bill Erickson --- diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Utils/HoldTargeter.pm b/Open-ILS/src/perlmods/lib/OpenILS/Utils/HoldTargeter.pm index b512bbc448..1f8e098161 100644 --- a/Open-ILS/src/perlmods/lib/OpenILS/Utils/HoldTargeter.pm +++ b/Open-ILS/src/perlmods/lib/OpenILS/Utils/HoldTargeter.pm @@ -42,30 +42,32 @@ sub new { # have either never been targeted or those whose prev_check_time exceeds # the retarget interval. # +# Returns an array of targeter response objects, one entry per hold +# targeted. See also return_count. +# # Optional parameters: # # hold => -# -- ID of a specific hold to target. +# (Re)target a specific hold. # # return_count => 1 -# -- If set, avoid collecting result objects for every hold processed -# and simply return the number of holds processed. This is useful -# for batch processing to avoid storing result data in memory for -# potentially hundreds of thousands of holds. +# Return the total number of holds processed instead of a result +# object for every targeted hold. Ideal for large batch targeting. # # retarget_interval => -# -- If set, this overrides the value found in the -# 'circ.holds.retarget_interval' global flag. +# Override the 'circ.holds.retarget_interval' global_flag value. # # newest_first => 1 -# -- If set, holds will be targeted in reverse order of create_time. -# This is useful for targeting / re-targeting newer holds first. +# Target holds in reverse order of create_time. # -# target_all => 1 -# -- Forces targeting / re-targeting of all active holds. -# This is primarily usefulf or testing. USE WITH CAUTION. +# retarget_nonviable => 1 +# Only retarget holds holds whose current_copy is no longer viable +# (e.g. copy was marked as non-circulate) or that have no current_copy +# set. This setting is ignored if 'hold' or 'retarget_all' are set. # -# Returns an array of hold targeter response objects, one response per hold. +# target_all => 1 +# USE WITH CAUTION. Forces (re)targeting of all active holds. This +# is primarily useful or testing. sub target { my ($self, %args) = @_; @@ -79,7 +81,10 @@ sub target { my @responses; for my $hold_id ($self->find_holds_to_target) { - my $single = OpenILS::Utils::HoldTargeter::Single->new(parent => $self); + my $single = OpenILS::Utils::HoldTargeter::Single->new( + parent => $self, + retarget_nonviable => $args{retarget_nonviable} + ); $single->target($hold_id); push(@responses, $single->result) unless $self->{return_count}; $count++; @@ -313,6 +318,12 @@ sub copy_prox_map { return $self->{copy_prox_map}; } +sub log_hold { + my ($self, $msg, $err) = @_; + my $level = $err ? 'error' : 'info'; + $logger->$level("targeter: [hold ".$self->hold_id."] $msg"); +} + # Captures the exit message, rolls back the cstore transaction/connection, # and returns false. # is_error : log the final message and editor event at ERR level. @@ -320,7 +331,7 @@ sub exit_targeter { my ($self, $msg, $is_error) = @_; $self->message($msg); - my $log = "targeter: exiting hold ".$self->hold_id." : $msg"; + my $log = "exiting => $msg"; if ($is_error) { # On error, roll back and capture the last editor event for logging. @@ -329,7 +340,7 @@ sub exit_targeter { $log .= " [".$evt->{textcode}."]" if $evt; $self->error(1); - $logger->error($log); + $self->log_hold($log, 1); } else { # Attempt a rollback and disconnect when each hold exits @@ -337,7 +348,7 @@ sub exit_targeter { # Note: ->rollback is a no-op when a ->commit has already occured. $self->editor->rollback; - $logger->info($log); + $self->log_hold($log); } return 0; @@ -527,8 +538,7 @@ sub get_hold_copies { my $copies = $e->json_query($query, {substream => 1}); $self->{eligible_copy_count} = scalar(@$copies); - $logger->info("targeter: Hold ".$self->hold_id." has ". - $self->{eligible_copy_count}." potential copies"); + $self->log_hold($self->{eligible_copy_count}." potential copies"); # Let the caller know we encountered the copy they were interested in. $self->{found_copy} = 1 if $self->{find_copy} @@ -609,7 +619,7 @@ sub filter_closed_date_copies { unless ($self->parent->get_ou_setting($clib, $ous)) { # Targeting not allowed at this circ lib when its closed - $logger->info("targeter: skipping copy ". + $self->log_hold("skipping copy ". $copy_hash->{id}."at closed org $clib"); next; @@ -672,6 +682,15 @@ sub inspect_previous_target { # previous target is no longer valid. return 1 unless $prev; + if ($self->{retarget_nonviable}) { + # In retarget_nonviable mode, leave the hold as-is if + # the existing current_copy is still permitted. + + return $self->exit_targeter( + "Skipping hold with viable target in 'retarget_nonviable' mode") + if $self->copy_is_permitted($prev); + } + # Previous copy is targetable. Keep it around for later. $self->{valid_previous_copy} = $prev; @@ -723,110 +742,114 @@ sub attempt_force_recall_target { return undef; } -# Copies at the pickup lib are allowed to bypass a lot of extra logic. -# See if we have any that are targetable. -sub attempt_local_copy_target { +sub attempt_to_find_copy { my $self = shift; - my $pu_lib = $self->hold->pickup_lib; - my @copies = @{$self->copies}; - my @locals = grep {$_->{circ_lib} eq $pu_lib} @copies; + return undef unless @{$self->copies}; - return undef unless @locals; + my $max_loops = $self->parent->get_ou_setting( + $self->hold->pickup_lib, + 'circ.holds.max_org_unit_target_loops' + ); - for my $copy (@locals) { - return $copy if $self->copy_is_permitted($copy); - } + return $self->target_by_org_loops($max_loops) if $max_loops; - $logger->info( - "targeter: no local targetable copies found for hold ".$self->hold_id); + # When not using target loops, targeting is based solely on + # proximity and org unit target weight. + $self->compile_weighted_proximity_map; - return undef; + return $self->find_nearest_copy; } -sub attempt_remote_copy_target { - my $self = shift; +# Returns 2 arrays. The first is a list of copies whose circ lib's +# unfulfilled target count matches the provided $iter value. The +# second list is all other copies, returned for convenience. +sub get_copies_at_loop_iter { + my ($self, $targeted_libs, $iter) = @_; - $logger->info("targeter: attempting to target a ". - "remote copy for hold ".$self->hold_id); + my @iter_copies; # copies to try now. + my @remaining_copies; # copies to try later - return undef unless @{$self->copies}; + for my $copy (@{$self->copies}) { + my $match = 0; - return undef unless $self->trim_copies_by_target_loop; + if ($iter == 0) { + # Start with copies at circ libs that have never been targeted. + $match = 1 unless grep { + $copy->{circ_lib} eq $_->{circ_lib}} @$targeted_libs; - $self->compile_weighted_proximity_map; + } else { + # Find copies at branches whose target count + # matches the current (non-zero) loop depth. - return $self->find_nearest_copy; -} - -sub trim_copies_by_target_loop { - my $self = shift; + $match = 1 if grep { + $_->{count} eq $iter && + $_->{circ_lib} eq $copy->{circ_lib} + } @$targeted_libs; + } - my $max_loops = $self->parent->get_ou_setting( - $self->hold->pickup_lib, - 'circ.holds.max_org_unit_target_loops' - ); + if ($match) { + push(@iter_copies, $copy); + } else { + push(@remaining_copies, $copy); + } + } - return 1 unless defined $max_loops; + $self->log_hold(scalar(@iter_copies). " potential copies with an ". + "unfulfilled circ_lib target attempt count of $iter"); - my @copies = @{$self->copies}; - my $e = $self->editor; + return (\@iter_copies, \@remaining_copies); +} - my %circ_lib_map = map {$_->{circ_lib} => 1} @copies; - my @circ_lib_list = keys %circ_lib_map; +# Find libs whose unfulfilled target count is less than the maximum +# configured loop count. Target copies in order of their circ_lib's +# target count (starting at 0) and moving up. Copies within each +# loop count group are weighted based on configured hold weight. If +# no copies in a given group are targetable, move up to the next +# unfulfilled target level. Keep doing this until all potential +# copies have been tried or max targets loops is exceeded. +# Returns a targetable copy if one is found, undef otherwise. +sub target_by_org_loops { + my ($self, $max_loops) = @_; + + my $targeted_libs = $self->editor->json_query({ + select => {aufhl => ['circ_lib', 'count']}, + from => 'aufhl', + where => {hold => $self->hold_id}, + order_by => [{class => 'aufhl', field => 'count'}] + }); - # Which target loop iteration are we currently on? - my $current_loop = $e->json_query({ - distinct => 1, - select => {aufhmxl => ['max']}, - from => 'aufhmxl', - where => {hold => $self->hold_id} - })->[0]; + $self->log_hold(scalar(@$targeted_libs). + " libs have been targeted at least once"); - $current_loop = $current_loop ? $current_loop->{max} : 1; + my $loop_iter = 0; + while ($loop_iter++ < $max_loops) { - # List of org units we've already tried targeting - # within the current target loop. - my $exclude_libs = $e->json_query({ - distinct => 1, - select => {aufhol => ['circ_lib']}, - from => 'aufhol', - where => {hold => $self->hold_id} - }); + my ($iter_copies, $remaining_copies) = + $self->get_copies_at_loop_iter($targeted_libs, $loop_iter); - my @keep_libs; - if (@$exclude_libs) { - my %exclude = map {$_->{circ_lib} => 1} @$exclude_libs; - for my $lib (@circ_lib_list) { - push(@keep_libs, $lib) unless $exclude{$lib}; - } - } else { - @keep_libs = @circ_lib_list; - } + next unless @$iter_copies; - # If we have exhausted every org unit within the current - # loop iteration, jump to the next loop iteration. - $current_loop++ unless @keep_libs; + $self->copies($iter_copies); - return $self->handle_exceeds_target_loops - if $current_loop > $max_loops; + # Update the proximity map to only include the copies + # from this loop-depth iteration. + $self->compile_weighted_proximity_map; - # Max hold loops not exceeded. Trim the set of targetable copies - # to those that at circ libs that have not been visited within - # the current target loop. + my $copy = $self->find_nearest_copy; + return $copy if $copy; # found one! - # New loop iteration. All copies are on the table. - return 1 unless @keep_libs; + # No targetable copy at the current target loop. + # Update our current copy set to the not-yet-tested copies. + $self->copies($remaining_copies); - my @new_copies; - for my $copy (@copies) { - push(@new_copies, $copy) - if grep {$copy->{circ_lib} eq $_} @keep_libs; + # We ran out of copies to try before exceeding max target loops. + # Nothing else to do here. + return undef unless @{$self->copies}; } - $self->copies(\@new_copies); - - return 1; + # Max target loops has been exceeded. + return $self->handle_exceeds_target_loops; } sub handle_exceeds_target_loops { @@ -856,18 +879,18 @@ sub handle_exceeds_target_loops { sub attempt_prev_copy_retarget { my $self = shift; - # attempt_remote_copy_target() can in some cases cancel the hold. + # earlier target logic can in some cases cancel the hold. return undef if $self->hold->cancel_time; my $prev_copy = $self->{valid_previous_copy}; return undef unless $prev_copy; - $logger->info("targeter: attempting to re-target ". - "previously targeted copy for hold ".$self->hold_id); + $self->log_hold("attempting to re-target previously ". + "targeted copy for hold ".$self->hold_id); if ($self->copy_is_permitted($prev_copy)) { - $logger->debug("targeter: retargeting the ". - "previously targeted copy [".$prev_copy->{id}."]" ); + $self->log_hold("retargeting the previously ". + "targeted copy [".$prev_copy->{id}."]" ); return $prev_copy; } @@ -958,8 +981,8 @@ sub log_unfulfilled_hold { return 1 unless my $prev_id = $self->{previous_copy_id}; my $e = $self->editor; - $logger->info("targeter: hold was not ". - "(but should have been) fulfilled by $prev_id"); + $self->log_hold( + "hold was not (but should have been) fulfilled by $prev_id"); my $circ_lib; if ($self->{valid_previous_copy}) { @@ -1015,7 +1038,7 @@ sub process_recalls { return unless $circ; - $logger->info("targeter: recalling circ ".$circ->id); + $self->log_hold("recalling circ ".$circ->id); # Give the user a new due date of either a full recall threshold, # or the return interval, whichever is further in the future. @@ -1044,7 +1067,7 @@ sub process_recalls { # If the OU hasn't defined new fine rules for recalls, keep them # as they were if ($fine_rules) { - $logger->info("targeter: applying recall fine rules: $fine_rules"); + $self->log_hold("applying recall fine rules: $fine_rules"); my $rules = OpenSRF::Utils::JSON->JSON2perl($fine_rules); $update_fields{recurring_fine} = $rules->[0]; $update_fields{fine_interval} = $rules->[1]; @@ -1073,7 +1096,7 @@ sub target { my $e = $self->editor; $self->hold_id($hold_id); - $logger->info("Processing hold $hold_id"); + $self->log_hold("processing..."); $e->xact_begin; @@ -1110,11 +1133,13 @@ sub target { return unless $self->handle_no_copies(process_recalls => 1); # At this point, the working list of copies has been trimmed to - # those that are currently targetable. + # those that are currently targetable at a superficial level. + # (They are holdable and available). Now the code steps through + # these copies in order of priority and pickup lib proximity to + # find a copy that is confirmed targetable by policy. my $copy = $self->attempt_force_recall_target || - $self->attempt_local_copy_target || - $self->attempt_remote_copy_target || + $self->attempt_to_find_copy || $self->attempt_prev_copy_retarget; # See if one of the above attempt* calls canceled the hold as a side