Re: bk commit into 6.0 tree (aelkin:1.2634) BUG#25998

[Mats Kindahl]

Andrei Elkin wrote:
> Mats, hello.
> 
> Just have committed a new cset which account remained from the last
> round issues.
> Besides, i changed IGNORE_SERVER_ID -> IGNORE_SERVER_IDS in the parser
> as the argument for the paramenter is a list.
> And there is one optimization for slave.cc's filtering out rule.
> I could not make it looking any simpler though.
> 
> 
> 
>>> 1. this bug issue voting agenda:
>>>
>>>    Shall we enforce that a new option to CHANGE MASTER will survive
>>>    reset master. I.e at the end of the following sequence of queries
>>>    
>>>    change master ignore_server_id=(1,2,3);
>>>    reset master;
>>>    start master;
>>>
>>>    wrt to the current IGNORE_SERVER_ID option we shall have either:
>>>
>>>    SHOW SLAVE STATUS like `ignore_server_id' => 1,2,3
>>>
>>>    or another alternative
>>>
>>>    SHOW SLAVE STATUS like `ignore_server_id' => empty
>> I vote that the list shall behave uniformly to how the other options
>> work: i.e., being retained over resets (without an intervening shutdown).
>>
> 
> I have mailed with explanations that I change my vote for persistency
> (of ignore_server_ids i.e :-).
> Therefore a hunk dealing with resetting the ignored list gets removed.
> Wrt to how else to reset the list, it's pretty simple - the empty rhs for
> IGNORE_SERVER_ID=().

OK. Good.

>>>>>>>   sql/rpl_mi.cc@stripped, 2008-05-08 21:00:32+03:00,
> aelkin@stripped +67 -4
>>>>>>>     initialization, storing into master info, reading from
> the file, deallocation
>>>>>>>     of the list of ignored server id:s.
>>>>>>>   
>>>>>> "initialization"? What did you do here?
>>>>>> "storing into master info"? Storing what into master info? What
> did
>>>>>> you do here?
>>>>>> "reading from the file"? What file? What did you do here?
>>>>>>
>>>>> I am lost in the number of questions...
>>>>>
>>>>> I seriously doubt you had a hard time to find out what is supposed to
> be
>>>>> stored into, read from the master info file :-)
>>>>> Besides, it's said explicitly at the end of the clause - "of the
> list
>>>>> of ignored server id:s". The latter is the common object of several
>>>>> operations mentioned in the comment clause.
>>>>>
>>>>> One point of your critique I have to accept:
>>>>> "initialization" meant allocation actually (of what is said at the
> end of the
>>>>> clause). (todo)
>>>>>
>>>>> Yes, there is yet another initialization. Of the new `master_id'
>>>>> member of Master_info.
>>>>> That's not said. Neither it is worth to mention as there is the
> source
>>>>> of the patch.
>>>>>
>>>>> "Reading from the file" means reading from a file that was last time
>>>>> mentioned.
>>>>>
>>>>> "What did you do here?" obviously, the slave reads from the master
>>>>> info file, the new line with the ignored server id list.
>>>> The point is that the comment is a fragment and does not make any sense
>>>> as it stands. I would prefer if you could write out the comment so that
>>>> it makes sense without having to look at the code.
>>> It's too strong requirement to provide comments eliminating need to
>>> look as the source code. Comments are by the definition a part of it.
>>> In my opinion, none, incl a reviewer, can demand such service as
>>> release from reading the sources.
>> You're nitpicking: the comment should be readable. It serves no purpose
>> otherwise when you browse the file history, which is what the comment is
>> used for.
> 
> The worst thing if we both would be nitpickers ...
> More than correcting "initialization" (of what) I have extended
> sql/rpl_mi.cc snippet of comments.

The problem was the fact that the comment was not comprehensible and not
any individual spelling or grammar, I don't view that as nitpicking.

> 
> Please find it in a new cset.

Yes, I noted it. It is much better.

> 
> 
>> [snip]
>>
>>>> I would say that it is expected that after master-slave.inc has been
>>>> executed, test is ready to run, including that the slave has started.
>>>>
>>>> I would prefer you could add that line to master-slave.inc instead.
>>> Well, two things.
>>>
>>> The main, i did not notice last time that it's not the test of SQL thread
>>>
>>>   include/wait_for_slave_to_start.inc 
>>>
>>> but synchronization with the master
>>>
>>>   sync_slave_with_master;
>>>
>>> which was meant, sorry.
>>>
>>> Second is a footnote{it would not be instead. "Instead" would be to do that
>>> in `start slave' handler because that suffice to
>>> `include/master-slave.inc', not vice versa}
>> Are you saying that you will add the line to master-slave.inc instead or
>> not?
> 
> I added the line with
> 
>    sync_slave_with_master
> 
> to include/master-slave.inc

OK.

Just my few cents,
Mats Kindahl

-- 
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com