maria-developers team mailing list archive

[andrei.elkin@xxxxxxxxxx]

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.

 2. Suggest to name them all with '_groups' suffix:

    DDL_groups, Non_transactiona_groups, Transactional_groups

    '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.
    
  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.

While the patch is certainly good,
I would like to have another look at the final patch to approve
formally.

Cheers,

Andrei


> 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&branch=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