Code review comment for lp:~percona-toolkit-dev/percona-toolkit/fix-1156901-skip-retry-check-for-repl-threads

Revision history for this message
Daniel Nichter (daniel-nichter) wrote :

46 + my $is_repl_thread = !$ms->is_replication_thread({
164 + my $is_repl_thread = $ms->is_replication_thread({

Looks like something wasn't updated?

Also, I think you can call it like: $ms->is_replication_thread($curr) since $curr is already the proclist hash.

51 + if ( !$is_repl_thread ) {
52 + PTDEBUG && _d('Query restarted; new query',
53 + $query_start, $etime, $prev->[START], $fudge);
54 + $new_query = 1;
55 + }
56 + elsif ( PTDEBUG ) {

Because we're checking if it's a repl thread, the same positive check first makes sense (else the code reads: "is it a repl thread? is it not a repl thread?"):

if ( $is_repl_thread ) {
   PTDEBUG && _d("Query has restarted but it's a replication thread, ignoring");
}
else {
   # Query has restarted but it's not a replication thread, so treat it as a new query.
   ...
   $new_query = 1;
}

57 + _d(q'Query has a start time beyond the fudge factor, ',
58 + q'but not counting it as a new query because, ',
59 + q{it's a replication thread});

Please always exercise consistency: we've never used q'' before, and it's confusing because ' is a quote char too; and the quoting changes in the same (multi) line to q{}.

review: Needs Fixing

« Back to merge proposal