maria-developers team mailing list archive

[Sachin Setiya <sachin.setiya@xxxxxxxxxxx>]

Hi Andrei!

On Mon, Jun 10, 2019 at 8:31 PM <andrei.elkin@xxxxxxxxxx> wrote:
>
> 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.]
Added a comment in code. Earlier it was unable to parse replication
filter with connection name
>
> >      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.
>
Sorry , Corrected.
> 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?
>
Right, Done
>
> > 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
Done, Sorry
>
> 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.
Done.
> > +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

Patch Link http://lists.askmonty.org/pipermail/commits/2019-June/013851.html

-- 
Regards
Sachin Setiya
Software Engineer at  MariaDB