Code review comment for lp:~percona-toolkit-dev/percona-toolkit/fix-950294-ptc-if-not-exists

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

1) "--no-create-replicate-table was passed in." -> ""--no-create-replicate-table was specified."

2)

29 + print STDERR "CREATE DATABASE IF NOT EXISTS $db failed, but the "
30 + . "db already exists. This is harmless unless the "
31 + . "slaves don't have the database. $EVAL_ERROR"; # kb link?

I think we need to make this fatal because we must avoid breaking replication. So:

die "Error executing $sql: $EVAL_ERROR\nThe database exists on the master, but replication will break if it does not also exist on all replicas. If the --replicate database and table already exist on the master and all replicas, or if you create them manually, then specify --no-create-replicate-table to avoid this error."

How does that ^ sound? Better to force the user to think/do a little manual work than break their replication.

3) 49 +<<<<<<< TREE -- why is there a merge conflict?

4) 63 + . "--no-create-replicate-table was passed in. " -- "passed id" -> "specified"

5) 73 + print STDERR "CREATE TABLE IF NOT EXISTS $args{repl_table} " -- Same as above with the db: make fatal, change error message, etc.

6) 142 +isnt($exit_status, 0, "");

i) Let's never use isnt() because 1) it amounts to "yes we have no bananas" ("yes, $got is not $bar") and 2) it's dangerously close to isn't() which is just crazy.

ii) Test has no name.

review: Needs Fixing

« Back to merge proposal