Code review comment for lp:~gl-az/percona-xtrabackup/bug1250375-2.1

Revision history for this message
Alexey Kopytov (akopytov) wrote :

> + if (!defined($log_bin_file) || !defined($log_bin_dir)) {
> + print STDERR "$now $prefix Failed to get master binlog " .
> + "directory and coordinates from SHOW MASTER STATUS\n";
> + print STDERR "$now $prefix This means that the server is not a " .
> + "member of a cluster nor a replication master. " .
> + "Ignoring the --galera-info option\n";
> +
>

The error message is wrong. Binary log must be enabled for GTID to work,
so failure to get binlog coordinates is an impossible condition. But it
certainly does not mean that the server is not a member of a
cluster. Nor does it mean that the server is not a replication master.

It does also not lead to the --galera-info option being ignored, because
we do write the galera info file in this case.

I would make it a fatal error. I.e.:

die “Failed to get binlog coordinates from SHOW MASTER STATUS”

[...]

> +if [[ -n ${WSREP_DEBUG:-} ]];then
> + start_server --log-bin=`hostname`-bin --binlog-format=ROW --wsrep-provider=${MYSQL_BASEDIR}/lib/libgalera_smm.so --wsrep_cluster_address=gcomm:// --wsrep-debug=1 --wsrep_provider_options="debug=1" --wsrep_node_address=$ADDR
> +else
> + start_server --log-bin=`hostname`-bin --binlog-format=ROW --wsrep-provider=${MYSQL_BASEDIR}/lib/libgalera_smm.so --wsrep_cluster_address=gcomm:// --wsrep_node_address=$ADDR
> +fi

gtid_mode=ON?

> +
> +
> +innobackupex --no-timestamp --galera-info $topdir/backup
> +backup_dir=$topdir/backup
> +vlog "Backup created in directory $backup_dir"
> +
> +count=0
> +master_file=
> +master_pos=
> +
> +while read line; do
> + if [ $count -eq 1 ] # File:
> + then
> + master_file=`echo "$line" | sed s/File://`
> + elif [ $count -eq 2 ] # Position:
> + then
> + master_pos=`echo "$line" | sed s/Position://`

master_pos is unused?

> + fi
> + count=$((count+1))
> +done <<< "`run_cmd $MYSQL $MYSQL_ARGS -Nse 'SHOW MASTER STATUS\G' mysql`"
> +
> +stop_server
> +
> +found_logs=`find $backup_dir -type f -name $master_file`

found_logs is unused? It should probably be used on the next line instead of $master_file, but why do you need to use 'find', i.e. recursive directory search? You could just use "if [ -f $backup_dir/$master_file ]" on the next line.

> +if [ -z "$master_file" ]
> +then
> + vlog "Could not find bin-log file in backup set, expecting to find $master_file"
> + exit 1
> +fi

We should be able to actualy run this test case in Jenkins. I pinged DS about adding galera56 to Jenkins matrix.

review: Needs Fixing

« Back to merge proposal