maria-developers team mailing list archive

[Sachin Setiya <sachin.setiya@xxxxxxxxxxx>]

Hi Andrei, Kristian !

Sorry for late reply (I was on vacation , and there was very less 10.3
sprint)


On Sat, Feb 10, 2018 at 5:42 PM, <andrei.elkin@xxxxxxxxxx> wrote:

> Sachin, hello.
>
>
> I've read through your latest patch as well as did so to the current
> mail thread.
> On previous rounds there were few notes raised by Kristian. I see that
> they got addressed.
>
> I got few requests of myself as we spoke yesterday
>
>  1. Let's count the transactional groups as well for consistency so that
>     the total count of all *taken* to execution groups would be gained
>     as simple as summing of the three kinds.
>
Done.

>
>  2. Suggest to name them all with '_groups' suffix:
>
>     DDL_groups, Non_transactiona_groups, Transactional_groups
>
Done, But I have added Slave as prefix.

>
>     'Event' is still defined as a sort of a statement, or the minimum
>      indivisible piece of work by a slave applier.  But the patch counts
>      Gtids that head any of the tree group of events, so it should be
>      the '_groups' suffix imo.  Sure, check with Ian (cc:d).
>      I see Kristian was somewhat sceptical about using 'group' as an
> offical
>      term, but that's the most natural way to name the object at hand.
>      For instace the transactional group is the same as just the
>      transaction (provided that `mysql.gtid_slave_pos` table is of the
>      same transactional type).
>
>   3. Make sure we document the counters are optimistic in sense they are
>      incremented in the beginning of a group execution before knowing
>      its outcome.
>
I am counting events in Gtid_log_event::do_apply_event , So counter is
optimistic.

>
>   4. Always head new [test] files with comments explaining top-level of
> the test content.
>      Specifcially to mention what is tested, and what are results of that.
>
Done.

>
> While the patch is certainly good,
> I would like to have another look at the final patch to approve
> formally.
>
> Cheers,
>
> Andrei
>
> Patch link
http://lists.askmonty.org/pipermail/commits/2018-March/012136.html
Regards
sachin

>
> > HI Kristian,
> >
> > On Thu, Jan 26, 2017 at 2:05 PM, Kristian Nielsen
> > <knielsen@xxxxxxxxxxxxxxx> wrote:
> >> Sachin Setiya <sachin.setiya@xxxxxxxxxxx> writes:
> >>
> >>>> Why did you decide to put this information into a status variable?
> >>>> Normally,
> >>>> slave status is seen in SHOW SLAVE STATUS, which supports showing
> >>>> status
> >>>> for
> >>>> a particular connection without using @@default_master_connection.
> >>>>
> >>>> Sorry for this , I guess It was a confusion I was looking at
> >>>> Slave_Running
> >>> this is avaliable in show status as well as
> >>> show slave status. So I thought for if I want to show some
> >>> information in
> >>> SHOW slave status this has to be in show status.
> >>> And this is wrong. Now we can see non trans events / ddl events in show
> >>> slave status. Show I remove this vars from Show Status, or it is okay ?
> >>
> >> But what do you think? What would you prefer, as a user? How about
> >> asking on
> >> the mailing list to get the input of actual users of the database,
> >> how they
> >> would use the feature and what they would prefer?
> >>
> >> I really worry that MariaDB development is deteriorated to the point
> that
> >> little thought goes into proper design anymore. It's all about "when
> >> can you
> >> push" whatever was assigned in Jira to your sprint. And little
> >> concern about
> >> _what_ is pushed.
> > No , this is only me. Other people are doing great work.
> >>
> >> I do not think the information should be duplicated in SHOW STATUS
> >> and SHOW
> >> SLAVE STATUS, so yes, one should be removed. SHOW SLAVE STATUS seems
> most
> >> appropriate, since it is naturally able to show per-master-connection
> >> information. Looking at the documentation for SHOW STATUS:
> >>
> >>   https://mariadb.com/kb/en/mariadb/show-status/
> >>
> >>   "With the GLOBAL modifier, SHOW STATUS displays the status values
> >> for all
> >>   connections to MariaDB. With SESSION, it displays the status values
> for
> >>   the current connection."
> >>
> >> Your patch makes SHOW STATUS display status for whatever is in
> >> @@default_master_connection (if I understand it correctly?). That seems
> >> inconsistent with how SHOW STATUS normally works.
> >>
> >>> Attached a newer version of patch , please review it.
> >>
> >>> diff --git a/mysql-test/suite/multi_source/multi_parallel.test
> >>> b/mysql-test/suite/multi_source/multi_parallel.test
> >>> new file mode 100644
> >>> index 0000000..b4f60e3
> >>> --- /dev/null
> >>> +++ b/mysql-test/suite/multi_source/multi_parallel.test
> >>
> >>> +--save_master_pos
> >>> +
> >>> +--connection slave
> >>> +--sync_with_master 0,'master2'
> >>> +show slave 'master2' status like 'Slave_DDL_Events';
> >>> +show slave 'master2' status like 'Slave_Non_Transactional_Events';
> >>
> >> I do not understand. This does not appear to be valid syntax. Your
> >> .result
> >> file does not have this, instead it has:
> >>
> >>> +set default_master_connection = 'master2';
> >>> +show status like 'Slave_DDL_Events';
> >>> +Variable_name        Value
> >>> +Slave_ddl_events     18
> >>> +show status like 'Slave_Non_Transactional_Events';
> >>> +Variable_name        Value
> >>> +Slave_non_transactional_events       36
> >>
> >> Do I misunderstand, or did you not even run your test case before
> >> sending it
> >> to me for review? If you did not, that simply is an unacceptable waste
> of
> >> the reviewer's time.
> >>
> > Sorry for this. I run test cases in pc and sent patch from laptop. Pc
> > was on older version ,
> > I just forgot to sync them :(. Sorry for wasting your time, next time
> > I will be more careful.
> >>> diff --git a/sql/slave.cc b/sql/slave.cc
> >>> index 1846938..0feace4 100644
> >>> --- a/sql/slave.cc
> >>> +++ b/sql/slave.cc
> >>> @@ -2823,6 +2823,15 @@ void show_master_info_get_fields(THD *thd,
> List<Item> *field_list,
> >>>                                              gtid_pos_length),
> >>>                            mem_root);
> >>>    }
> >>> +  field_list->push_back(new (mem_root)
> >>> +                       Item_return_int(thd, "Slave_DDL_Events", 20,
> >>> +                                       MYSQL_TYPE_LONGLONG),
> >>> +                       mem_root);
> >>> +
> >>> +  field_list->push_back(new (mem_root)
> >>> +                       Item_return_int(thd,
> "Slave_Non_Transactional_Events", 20,
> >>> +                                       MYSQL_TYPE_LONGLONG),
> >>> +                       mem_root);
> >>>    DBUG_VOID_RETURN;
> >>>  }
> >>
> >> If I read this correctly, you seem to have put the new SHOW SLAVE STATUS
> >> fields _after_ the extra fields output with the ALL SLAVES
> >> option. Doesn't
> >> that seem a bit messy? Other new options were added _before_ those of
> the
> >> ALL SLAVES option. See also this mailing list discussion:
> >>
> >>   https://lists.launchpad.net/maria-developers/msg09993.html
> >>
> > Changed. Yes you are right.
> >> Also, this seems to be based on MariaDB-10.1 code (since 10.2 has
> >> Slave_SQL_Running_State at this point)? A new feature like this
> >> shouldn't go
> >> into 10.1.
> >>
> > Ported the patch to 10.2. Yes you are right I am doing coding in very
> > bad way , just
> > following the jira task :( .
> >>  - Kristian.
> > If you like to review , I have included a newer version of patch. This
> > time I have tested it on
> > pc. http://lists.askmonty.org/pipermail/commits/2017-January/010589.html
> >
> > Buildbot link:-
> > https://buildbot.askmonty.org/buildbot/grid?category=main&br
> anch=bb-10.2-sachin
> >
> > Regards
> > sachin
> >
> > _______________________________________________
> > Mailing list: https://launchpad.net/~maria-developers
> > Post to     : maria-developers@xxxxxxxxxxxxxxxxxxx
> > Unsubscribe : https://launchpad.net/~maria-developers
> > More help   : https://help.launchpad.net/ListHelp
>



-- 
Regards
Sachin Setiya
Software Engineer at  MariaDB