maria-developers team mailing list archive

[Sachin Setiya <sachin.setiya@xxxxxxxxxxx>]

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