From a25b9eab40781754e99a5bbf5b42d2d71427229b Mon Sep 17 00:00:00 2001
From: "Brian Fraser fraserb@gmail.com" <>
Date: Wed, 1 Aug 2012 17:39:12 -0300
Subject: [PATCH] MasterSlave: Rework how the recursion methods are checked &
 resolved

In part by removing the OptionParser usage out from get_slaves
and recurse_to_slaves and making them expect arrayrefs, thus
forcing our callers to deal with that, and in part by
splitting out the method-checking to MasterSlave::check_recursion_method
and the resolving (originally in find_slave_hosts) into
_resolve_recursion_methods.
---
 lib/MasterSlave.pm  | 82 ++++++++++++++++++++++++++++-----------------
 t/lib/MasterSlave.t | 65 +++++++++++++++++++++++++++++------
 2 files changed, 106 insertions(+), 41 deletions(-)

diff --git a/lib/MasterSlave.pm b/lib/MasterSlave.pm
index d343e52c..341302e6 100644
--- a/lib/MasterSlave.pm
+++ b/lib/MasterSlave.pm
@@ -38,27 +38,29 @@ sub new {
 
 sub get_slaves {
    my ($self, %args) = @_;
-   my @required_args = qw(make_cxn OptionParser DSNParser Quoter);
+   my @required_args = qw(make_cxn recursion_method recurse DSNParser Quoter);
    foreach my $arg ( @required_args ) {
-      die "I need a $arg argument" unless $args{$arg};
+                                     # exists because recurse can be undef
+      die "I need a $arg argument" unless exists $args{$arg};
    }
-   my ($make_cxn, $o, $dp) = @args{@required_args};
+   my ($make_cxn, $methods, $recurse, $dp) = @args{@required_args};
 
    my $slaves = [];
-   my $method = $o->get('recursion-method');
-   PTDEBUG && _d('Slave recursion method:', $method);
-   if ( !$method || $method =~ m/processlist|hosts/i ) {
+   
+   PTDEBUG && _d('Slave recursion method:', $methods);
+   if ( !@$methods || grep { m/processlist|hosts/i } @$methods ) {
       my @required_args = qw(dbh dsn);
       foreach my $arg ( @required_args ) {
          die "I need a $arg argument" unless $args{$arg};
       }
       my ($dbh, $dsn) = @args{@required_args};
+      
       $self->recurse_to_slaves(
          {  dbh        => $dbh,
             dsn        => $dsn,
             dsn_parser => $dp,
-            recurse    => $o->get('recurse'),
-            method     => $o->get('recursion-method'),
+            recurse    => $recurse,
+            method     => $methods,
             callback   => sub {
                my ( $dsn, $dbh, $level, $parent ) = @_;
                return unless $level;
@@ -69,25 +71,44 @@ sub get_slaves {
          }
       );
    }
-   elsif ( $method =~ m/^dsn=/i ) {
-      my ($dsn_table_dsn) = $method =~ m/^dsn=(.+)/i;
+   elsif ( @$methods && $methods->[0] =~ m/^dsn=/i ) {
+      (my $dsn_table_dsn = join ",", @$methods) =~ s/^dsn=//i;
       $slaves = $self->get_cxn_from_dsn_table(
          %args,
          dsn_table_dsn => $dsn_table_dsn,
       );
    }
-   elsif ( $method =~ m/none/i ) {
-      # https://bugs.launchpad.net/percona-toolkit/+bug/987694
+   elsif ( !@$methods || $methods->[0] =~ m/none/i ) {
       PTDEBUG && _d('Not getting to slaves');
    }
    else {
-      die "Invalid --recursion-method: $method.  Valid values are: "
-        . "dsn=DSN, hosts, or processlist.\n";
+      die "Unexpected recursion methods: @$methods";
    }
-
+   
    return $slaves;
 }
 
+# Sub: check_recursion_method
+#   Check that the arrayref of recursion methods passed in is valid
+sub check_recursion_method {
+   my ($methods) = @_;
+
+   if ( @$methods != 1 ) {
+      if ( grep({ !m/processlist|hosts/i } @$methods)
+            && $methods->[0] !~ /^dsn=/i )
+      {
+         die  "Invalid combination of recursion methods: "
+            . join(", ", map { defined($_) ? $_ : 'undef' } @$methods) . ". "
+            . "Only hosts and processlist may be combined.\n"
+      }
+   }
+   else {
+      my ($method) = @$methods;
+      die "Invalid recursion method: " . ( $method || 'undef' )
+         unless $method && $method =~ m/^(?:processlist$|hosts$|none$|dsn=)/i;
+   }
+}
+
 # Sub: recurse_to_slaves
 #   Descend to slaves by examining SHOW SLAVE HOSTS.
 #   The callback gets the slave's DSN, dbh, parent, and the recursion level
@@ -114,7 +135,7 @@ sub recurse_to_slaves {
    my $dp   = $args->{dsn_parser};
    my $dsn  = $args->{dsn};
 
-   if ( lc($args->{method} || '') eq 'none' ) {
+   if ( $args->{method} && lc($args->{method}->[0] || '') eq 'none' ) {
       # https://bugs.launchpad.net/percona-toolkit/+bug/987694
       PTDEBUG && _d('Not recursing to slaves');
       return;
@@ -184,21 +205,10 @@ sub recurse_to_slaves {
 # If a method is given, it becomes the preferred (first tried) method.
 # Searching stops as soon as a method finds slaves.
 sub find_slave_hosts {
-   my ( $self, $dsn_parser, $dbh, $dsn, $method ) = @_;
+   my ( $self, $dsn_parser, $dbh, $dsn, $methods ) = @_;
+
+   my @methods = $self->_resolve_recursion_methods($methods, $dsn);
 
-   my @methods = qw(processlist hosts);
-   if ( $method ) {
-      # Remove all but the given method.
-      @methods = grep { $_ ne $method } @methods;
-      # Add given method to the head of the list.
-      unshift @methods, $method;
-   }
-   else {
-      if ( ($dsn->{P} || 3306) != 3306 ) {
-         PTDEBUG && _d('Port number is non-standard; using only hosts method');
-         @methods = qw(hosts);
-      }
-   }
    PTDEBUG && _d('Looking for slaves on', $dsn_parser->as_string($dsn),
       'using methods', @methods);
 
@@ -215,6 +225,18 @@ sub find_slave_hosts {
    return @slaves;
 }
 
+sub _resolve_recursion_methods {
+   my ($self, $methods, $dsn) = @_;
+
+   # If an explicit recursion method was specified, use that
+   return @$methods if $methods && @$methods;
+   # Otherwise, if we're on the standard port, default to processlist and hosts
+   return qw( processlist hosts ) if (($dsn->{P} || 3306) == 3306);
+   # Or if on a non-standard port, default to hosts.
+   PTDEBUG && _d('Port number is non-standard; using only hosts method');
+   return qw( hosts );
+}
+
 sub _find_slaves_by_processlist {
    my ( $self, $dsn_parser, $dbh, $dsn ) = @_;
 
diff --git a/t/lib/MasterSlave.t b/t/lib/MasterSlave.t
index 8207bd1f..7b1a76de 100644
--- a/t/lib/MasterSlave.t
+++ b/t/lib/MasterSlave.t
@@ -9,7 +9,7 @@ BEGIN {
 use strict;
 use warnings FATAL => 'all';
 use English qw(-no_match_vars);
-use Test::More tests => 52;
+use Test::More;
 
 use MasterSlave;
 use DSNParser;
@@ -20,6 +20,8 @@ use Cxn;
 use Sandbox;
 use PerconaTest;
 
+use Data::Dumper;
+
 my $ms = new MasterSlave();
 my $dp = new DSNParser(opts=>$dsn_opts);
 my $sb = new Sandbox(basedir => '/tmp', DSNParser => $dp);
@@ -43,13 +45,16 @@ $o->get_specs("$trunk/bin/pt-table-checksum");
 
 SKIP: {
    skip "Cannot connect to sandbox master", 2 unless $master_dbh;
-   @ARGV = ();
+   local @ARGV = ();
    $o->get_opts();
-
+   
    my $slaves = $ms->get_slaves(
       dbh          => $master_dbh,
       dsn          => $master_dsn,
-      OptionParser => $o,
+      recursion_method => $o->got('recursion-method')
+                        ? $o->get('recursion-method')
+                        : [],
+      recurse      => $o->get('recurse'),
       DSNParser    => $dp,
       Quoter       => $q,
       make_cxn     => sub {
@@ -78,7 +83,7 @@ SKIP: {
          master_id => 12345,
          source    => 'hosts',
       },
-      'get_slaves() from recurse_to_slaves()'
+      'get_slaves() from recurse_to_slaves() with a default --recursion-method works'
    );
 
    my ($id) = $slaves->[0]->dbh()->selectrow_array('SELECT @@SERVER_ID');
@@ -93,11 +98,14 @@ SKIP: {
    # and ignore it.  This tests nonetheless that "processlist" isn't
    # misspelled, which would cause the sub to die.
    # https://bugs.launchpad.net/percona-toolkit/+bug/921802
-   @ARGV = ('--recursion-method', 'processlist');
+   local @ARGV = ('--recursion-method', 'processlist');
    $o->get_opts();
 
    $slaves = $ms->get_slaves(
-      OptionParser => $o,
+      recursion_method => $o->got('recursion-method')
+                        ? $o->get('recursion-method')
+                        : [],
+      recurse      => $o->get('recurse'),
       DSNParser    => $dp,
       Quoter       => $q,
       dbh          => $master_dbh,
@@ -146,7 +154,10 @@ SKIP: {
    throws_ok(
       sub {
          $slaves = $ms->get_slaves(
-            OptionParser => $o,
+            recursion_method => $o->got('recursion-method')
+                              ? $o->get('recursion-method')
+                              : [],
+            recurse      => $o->get('recurse'),
             DSNParser    => $dp,
             Quoter       => $q,
             dbh          => $ro_dbh,
@@ -169,7 +180,10 @@ SKIP: {
    @ARGV = ('--recursion-method', 'none');
    $o->get_opts();
    $slaves = $ms->get_slaves(
-      OptionParser => $o,
+      recursion_method => $o->got('recursion-method')
+                        ? $o->get('recursion-method')
+                        : [],
+      recurse      => $o->get('recurse'),
       DSNParser    => $dp,
       Quoter       => $q,
       dbh          => $ro_dbh,
@@ -671,7 +685,10 @@ $sb->load_file('master', "t/lib/samples/MasterSlave/dsn_table.sql");
 $o->get_opts();
 
 my $slaves = $ms->get_slaves(
-   OptionParser => $o,
+   recursion_method => $o->got('recursion-method')
+                     ? $o->get('recursion-method')
+                     : [],
+   recurse      => $o->get('recurse'),
    DSNParser    => $dp,
    Quoter       => $q,
    make_cxn     => sub {
@@ -707,6 +724,31 @@ is(
    'dbh created from DSN table works'
 );
 
+# ############################################################################
+# Invalid recursion methods are caught
+# ############################################################################
+local $EVAL_ERROR;
+eval {
+   MasterSlave::check_recursion_method([qw(stuff)])
+};
+
+like(
+   $EVAL_ERROR,
+   qr/Invalid recursion method: stuff/,
+   "Invalid recursion methods are caught",
+);
+
+local $EVAL_ERROR;
+eval {
+   MasterSlave::check_recursion_method([qw(processlist stuff)])
+};
+
+like(
+   $EVAL_ERROR,
+   qr/Invalid combination of recursion methods: processlist, stuff/,
+   "Invalid recursion methods are caught",
+);
+
 # #############################################################################
 # Done.
 # #############################################################################
@@ -716,4 +758,5 @@ diag(`/tmp/12346/use -e "set global read_only=1"`);
 diag(`/tmp/12347/use -e "set global read_only=1"`);
 diag(`$trunk/sandbox/test-env reset >/dev/null 2>&1`);
 ok($sb->ok(), "Sandbox servers") or BAIL_OUT(__FILE__ . " broke the sandbox");
-exit;
+
+done_testing;