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.el...@pp.inet.fi> 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 > > <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&br > anch=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 > -- Regards Sachin Setiya Software Engineer at MariaDB
_______________________________________________ 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
