maria-developers team mailing list archive

Thread
Date
Re: [Commits] 321a43d9ac5: MDEV-8874 Replication filters configured in my.cnf are ignored if slave reset and reconfigured

To: sachin.setiya@xxxxxxxxxxx
From: andrei.elkin@xxxxxxxxxx
Date: Mon, 10 Jun 2019 18:01:04 +0300
Cc: commits@xxxxxxxxxxx, maria-developers@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20190606064431.A5C431360204@slap.localdomain> (sachin setiya's message of "Thu, 6 Jun 2019 12:14:31 +0530 (IST)")
Organization: Home sweet home
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux)
Sachin, salute!

The patch looks good, but I think it misses some verbosity in few
places, please read on.

> revision-id: 321a43d9ac586034790407dc519c72e90b3bff81 (mariadb-10.1.39-54-g321a43d9ac5)
> parent(s): b003b0c934cf6b59358e31144c4f69cf34622fb8
> author: Sachin
> committer: Sachin
> timestamp: 2019-06-06 12:03:45 +0530
> message:
>
> MDEV-8874 Replication filters configured in my.cnf are ignored if slave reset and reconfigured
>
> Don't delete the rpl_filter on RESET SLAVE.
>
> ---
>  mysql-test/lib/My/Config.pm                    |   8 +-
>  mysql-test/suite/multi_source/mdev-8874.cnf    |  25 +++++
>  mysql-test/suite/multi_source/mdev-8874.result |  93 +++++++++++++++++++
>  mysql-test/suite/multi_source/mdev-8874.test   | 123 +++++++++++++++++++++++++
>  sql/rpl_mi.cc                                  |   2 -
>  5 files changed, 248 insertions(+), 3 deletions(-)
>
> diff --git a/mysql-test/lib/My/Config.pm b/mysql-test/lib/My/Config.pm
> index 12647edf0a4..349a397ed6d 100644
> --- a/mysql-test/lib/My/Config.pm
> +++ b/mysql-test/lib/My/Config.pm
> @@ -327,7 +327,13 @@ sub new {
>        # Skip comment
>        next;
>      }
> -    
> +
> +    # Replication Filter
> +    elsif ( $line =~ /^([\w]+.[\w]+)\s*=\s*(.*)\s*/){
> +       my $option= $1;
> +       my $value= $2;
> +       $self->insert($group_name, $option, $value);
> +    }

What was a problem at the above point?
[Anything that you think may not be straightforward for a patch reader is
always worth to highlight in the description.]

>      else {
>        croak "Unexpected line '$line' found in '$path'";
>      }
> diff --git a/mysql-test/suite/multi_source/mdev-8874.cnf b/mysql-test/suite/multi_source/mdev-8874.cnf
> new file mode 100644
> index 00000000000..01da70cbaa0
> --- /dev/null
> +++ b/mysql-test/suite/multi_source/mdev-8874.cnf
> @@ -0,0 +1,25 @@
> +!include my.cnf
> +
> +[mysqld.1]
> +log-bin
> +log-slave-updates
> +
> +[mysqld.2]
> +log-bin
> +log-slave-updates
> +
> +[mysqld.3]
> +log-bin
> +log-slave-updates
> +
> +[mysqld.4]
> +server-id=4
> +log-bin=server4-bin
> +log-slave-updates
> +m1.replicate_ignore_table='a.t1'
> +m2.replicate_ignore_table='b.t1'
> +replicate_ignore_table='c.t1'
> +
> +[ENV]
> +SERVER_MYPORT_4=		@mysqld.4.port
> +SERVER_MYSOCK_4=		@mysqld.4.socket
> diff --git a/mysql-test/suite/multi_source/mdev-8874.result b/mysql-test/suite/multi_source/mdev-8874.result
> new file mode 100644
> index 00000000000..6fb364a3d2d
> --- /dev/null
> +++ b/mysql-test/suite/multi_source/mdev-8874.result
> @@ -0,0 +1,93 @@
> +create database a;
> +use a;
> +create table t1(a int);
> +insert into t1 values(1);
> +create table t2(a int);
> +insert into t2 values(1);
> +create database b;
> +use b;
> +create table t1(a int);
> +insert into t1 values(1);
> +create table t2(a int);
> +insert into t2 values(1);
> +create database c;
> +use c;
> +create table t1(a int);
> +insert into t1 values(1);
> +create table t2(a int);
> +insert into t2 values(1);
> +change master 'm1' to master_port=MYPORT_1 , master_host='127.0.0.1', master_user='root';
> +change master 'm2' to master_port=MYPORT_2 , master_host='127.0.0.1', master_user='root';
> +change master  to master_port=MYPORT_3 , master_host='127.0.0.1', master_user='root';
> +start all slaves;
> +set default_master_connection = 'm1';
> +include/wait_for_slave_to_start.inc
> +set default_master_connection = 'm2';
> +include/wait_for_slave_to_start.inc
> +set default_master_connection = '';
> +include/wait_for_slave_to_start.inc
> +use a;
> +show tables;
> +Tables_in_a
> +t2
> +use b;
> +show tables;
> +Tables_in_b
> +t2
> +use c;
> +show tables;
> +Tables_in_c
> +t2
> +#TEST
> +STOP ALL SLAVES;
> +Warnings:
> +Note	1938	SLAVE 'm2' stopped
> +Note	1938	SLAVE '' stopped
> +Note	1938	SLAVE 'm1' stopped
> +RESET SLAVE 'm1' ALL ;
> +RESET SLAVE 'm2' ALL ;
> +RESET SLAVE ALL ;
> +drop database a;
> +drop database b;
> +drop database c;
> +change master 'm1' to master_port=MYPORT_1 , master_host='127.0.0.1', master_user='root';
> +change master 'm2' to master_port=MYPORT_2 , master_host='127.0.0.1', master_user='root';
> +change master  to master_port=MYPORT_3 , master_host='127.0.0.1', master_user='root';
> +start all slaves;
> +Warnings:
> +Note	1937	SLAVE 'm2' started
> +Note	1937	SLAVE '' started
> +Note	1937	SLAVE 'm1' started
> +set default_master_connection = 'm1';
> +include/wait_for_slave_to_start.inc
> +set default_master_connection = 'm2';
> +include/wait_for_slave_to_start.inc
> +set default_master_connection = '';
> +include/wait_for_slave_to_start.inc
> +use a;
> +show tables;
> +Tables_in_a
> +t2
> +use b;
> +show tables;
> +Tables_in_b
> +t2
> +use c;
> +show tables;
> +Tables_in_c
> +t2
> +#CleanUp
> +drop database a;
> +drop database b;
> +drop database c;
> +stop all slaves;
> +Warnings:
> +Note	1938	SLAVE 'm2' stopped
> +Note	1938	SLAVE '' stopped
> +Note	1938	SLAVE 'm1' stopped
> +SET default_master_connection = "m1";
> +include/wait_for_slave_to_stop.inc
> +SET default_master_connection = "m2";
> +include/wait_for_slave_to_stop.inc
> +SET default_master_connection = "";
> +include/wait_for_slave_to_stop.inc

To the test part, I suggest we always follow a well-know description pattern (which
is a great time saver when it comes to future test maintenance, apart
from the current reviewing), to include the task id (present in the name
though) and title, what failed and *how* the test proves the fixes.

I can bet the failure was also exposed by reset
@@global.'connection_name'.'rule_type' variables, which the test is
really missing. Could you please add up that?


> diff --git a/mysql-test/suite/multi_source/mdev-8874.test b/mysql-test/suite/multi_source/mdev-8874.test
> new file mode 100644
> index 00000000000..b6d1aafe139
> --- /dev/null
> +++ b/mysql-test/suite/multi_source/mdev-8874.test
> @@ -0,0 +1,123 @@
> +--source include/not_embedded.inc
> +--source include/have_innodb.inc
> +--source include/have_debug.inc
> +
> +--connect (server_1,127.0.0.1,root,,,$SERVER_MYPORT_1)
> +--connect (server_2,127.0.0.1,root,,,$SERVER_MYPORT_2)
> +--connect (server_3,127.0.0.1,root,,,$SERVER_MYPORT_3)
> +--connect (server_4,127.0.0.1,root,,,$SERVER_MYPORT_4)
> +
> +--connection server_1
> +create database a;
> +use a;
> +create table t1(a int);
> +insert into t1 values(1);
> +create table t2(a int);
> +insert into t2 values(1);
> +--save_master_pos
> +
> +--connection server_2
> +create database b;
> +use b;
> +create table t1(a int);
> +insert into t1 values(1);
> +create table t2(a int);
> +insert into t2 values(1);
> +--save_master_pos
> +
> +--connection server_3
> +create database c;
> +use c;
> +create table t1(a int);
> +insert into t1 values(1);
> +create table t2(a int);
> +insert into t2 values(1);
> +--save_master_pos
> +
> +--connection server_4
> +--disable_warnings
> +--replace_result $SERVER_MYPORT_1 MYPORT_1
> +eval change master 'm1' to master_port=$SERVER_MYPORT_1 , master_host='127.0.0.1', master_user='root';
> +--replace_result $SERVER_MYPORT_2 MYPORT_2
> +eval change master 'm2' to master_port=$SERVER_MYPORT_2 , master_host='127.0.0.1', master_user='root';
> +--replace_result $SERVER_MYPORT_3 MYPORT_3
> +eval change master  to master_port=$SERVER_MYPORT_3 , master_host='127.0.0.1', master_user='root';
> +start all slaves;
> +set default_master_connection = 'm1';
> +--source include/wait_for_slave_to_start.inc
> +set default_master_connection = 'm2';
> +--source include/wait_for_slave_to_start.inc
> +set default_master_connection = '';
> +--source include/wait_for_slave_to_start.inc
> +
> +--enable_warnings
> +--sync_with_master 0,'m1'
> +--sync_with_master 0,'m2'
> +--sync_with_master 0,''
> +use a;

When the test proves something via printing out, it's always good to
prepare the output with an

  --echo what-is-expected

so a thankful reader will bless you .. and the others may mercy :-)

> +show tables;
> +use b;
> +show tables;
> +use c;
> +show tables;
> +--echo #TEST
> +STOP ALL SLAVES;
> +RESET SLAVE 'm1' ALL ;
> +RESET SLAVE 'm2' ALL ;
> +RESET SLAVE ALL ;
> +drop database a;
> +drop database b;
> +drop database c;
> +--replace_result $SERVER_MYPORT_1 MYPORT_1
> +eval change master 'm1' to master_port=$SERVER_MYPORT_1 , master_host='127.0.0.1', master_user='root';
> +--replace_result $SERVER_MYPORT_2 MYPORT_2
> +eval change master 'm2' to master_port=$SERVER_MYPORT_2 , master_host='127.0.0.1', master_user='root';
> +--replace_result $SERVER_MYPORT_3 MYPORT_3
> +eval change master  to master_port=$SERVER_MYPORT_3 , master_host='127.0.0.1', master_user='root';
> +start all slaves;
> +set default_master_connection = 'm1';
> +--source include/wait_for_slave_to_start.inc
> +set default_master_connection = 'm2';
> +--source include/wait_for_slave_to_start.inc
> +set default_master_connection = '';
> +--source include/wait_for_slave_to_start.inc
> +--sync_with_master 0,'m1'
> +--sync_with_master 0,'m2'
> +--sync_with_master 0,''
> +

Naturally, the same applies to the following block.
> +use a;
> +show tables;
> +use b;
> +show tables;
> +use c;
> +show tables;

> +
> +
> +#--echo #restart the server
> +#--source include/restart_mysqld.inc
> +
> +
> +--echo #CleanUp
> +--connection server_1
> +drop database a;
> +--save_master_pos
> +
> +--connection server_2
> +drop database b;
> +--save_master_pos
> +
> +--connection server_3
> +drop database c;
> +--save_master_pos
> +
> +--connection server_4
> +--sync_with_master 0,'m1'
> +--sync_with_master 0,'m2'
> +--sync_with_master 0,''
> +stop all slaves;
> +SET default_master_connection = "m1";
> +--source include/wait_for_slave_to_stop.inc
> +SET default_master_connection = "m2";
> +--source include/wait_for_slave_to_stop.inc
> +SET default_master_connection = "";
> +--source include/wait_for_slave_to_stop.inc
> diff --git a/sql/rpl_mi.cc b/sql/rpl_mi.cc
> index 70e60b1d4ad..7aea89337a7 100644
> --- a/sql/rpl_mi.cc
> +++ b/sql/rpl_mi.cc
> @@ -122,8 +122,6 @@ Master_info::~Master_info()
>    */
>    if (strncmp(connection_name.str, STRING_WITH_LEN("wsrep")))
>  #endif
> -  rpl_filters.delete_element(connection_name.str, connection_name.length,
> -                             (void (*)(const char*, uchar*)) free_rpl_filter);
>    my_free(connection_name.str);
>    delete_dynamic(&ignore_server_ids);
>    mysql_mutex_destroy(&run_lock);
> _______________________________________________
> commits mailing list
> commits@xxxxxxxxxxx
> https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
Follow ups

Re: [Commits] 321a43d9ac5: MDEV-8874 Replication filters configured in my.cnf are ignored if slave reset and reconfigured
From: Sachin Setiya, 2019-06-12