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

[Andrei Elkin]

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=().


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

Please find it in a new cset.


>
> [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

cheers,

Andrei