HI Kristian, On Thu, Jan 26, 2017 at 2:05 PM, Kristian Nielsen <kniel...@knielsen-hq.org> wrote: > Sachin Setiya <sachin.set...@mariadb.com> 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@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
