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.
2025-09-10 21:19:59 +00:00 · 2012-08-01 17:39:12 -03:00
parent 192f38498c
commit a25b9eab40
2 changed files with 106 additions and 41 deletions
--- 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 ) = @_;

--- 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;