Hi Andrei,
I see no remaining problems that you have not accepted to fix. Once you
have a new patch, I can approve it.
Best wishes,
Mats Kindahl
Andrei Elkin wrote:
> Mats, hello.
>
>> Hi Andrei!
>>
>> Patch is a lot better, but I still have comments on it. There are no
>> fundamental issues this time, but rather details about the implementation.
>
> I understand. This fix is a WL size of work and some efforts had to be
> spend on HLD.
>
> I am accepting your comment on using the standard function.
> Please find inlined my answers to your other notes as well.
[snip]
>>> +reset slave;
>>> +*** must be empty due to reset slave ***
>>> +ignore server id list: 10, 100
>>> +change master to IGNORE_SERVER_IDS= (10, 100);
>> Comment here is not correct: the list is not empty
>>
>
> Sigh, you must have reviewed not the latest commit...
>
> http://lists.mysql.com/commits/47357 has
>
> +change master to IGNORE_SERVER_IDS= (10, 100);
> +*** the list must remain (10, 100) after reset slave ***
> +change master to IGNORE_SERVER_IDS= ();
> +*** must be empty due to IGNORE_SERVER_IDS empty list ***
I took the one I saw with the latest time...
[snip]
>> Where is the check that list is the same?
>>
>
> It's in the latest commit.
OK
>
>>> +change master to IGNORE_SERVER_IDS= ();
>> Normally, when one tests a function with lists, usually lengths 0, 1, 2,
>> and 3 are tested. That means that the most problematic cases are tested,
>> i.e.:
>>
>> - there is an empty list
>>
>> - there is a list where an element is both first and last
>>
>> - there is a list where an element is first and one element is last,
>> but they are not the same element
>>
>> - there is a case where an element is neither first nor last
>>
>>> +*** must be empty due to IGNORE_SERVER_IDS empty list ***
>>> +ignore server id list:
>
> It's easy to add what you are asking. Still, i think such testing
> would be for the parser's basic funcionality. The patch does not
> introduce any new list operation explicitly.
You're using the ignore_server_ids list internally, that is what you
should check, not that the parser can read the list correctly.
However, it's your choice, I see no problems with the current code, it's
just that it might change in the future (out of your control), and
having tests for such basic properties usually helps a lot in preventing
regressions.
[snip]
>> I don't see a test for a list with duplicate server id:s, for example,
>> (1, 1, 1, 3, 5). Could you please add such a case and show what is the
>> result of that setting.
>>
>> Also, what about giving server id's in random order, e.g.: (4, 2, 1, 5)?
>
> sure
[snip]
>>> +my_bool dynamic_array_ulong_bin_search(DYNAMIC_ARRAY *array, const ulong
> *search,
>>> + uint * index)
>>> +{
>>> + DBUG_ENTER("dynamic_array_bin_search");
>>> + long l= 0, i, r= array->elements - 1;
>>> + while (l <= r)
>>> + {
>>> + ulong found;
>>> + DBUG_ASSERT((r < 0 || (ulong) r < array->elements)
> && l >=0);
>>> + i= (r + l)/2;
>>> + if ((found= (* (ulong*) dynamic_array_ptr(array, i))) == *search)
>>> + {
>>> + if (index)
>>> + *index= i;
>>> + DBUG_RETURN(FALSE);
>>> + }
>>> + if (found < *search)
>>> + l= i + 1;
>>> + else
>>> + r= i - 1;
>>> + }
>>> + DBUG_RETURN(TRUE);
>>> +}
>> Why not use the standard bsearch() function available in all known
>> compiler implementations? The dynamic array is designed to support that
>> usage.
>>
>
> I really missed that's the standard C lib ...
> Sure, we are better to switch to the standard.
Good.
>
>>> +
>>> +/**
>>> + Reports if the s_id server has been configured to ignore events
>>> + it generates with
>>> +
>>> + CHANGE MASTER IGNORE_SERVER_IDS= ( list of server ids )
>>> +
>>> + Method is called from the io thread event receiver filtering.
>>> +
>>> + @param s_id the master server identifier
>>> + @return
>>> + @retval TRUE if s_id is in the list of ignored master servers,
>>> + @retval FALSE otherwise.
>>> + */
>>> +bool Master_info::repl_server_id_ignore_p(ulong s_id)
>> Please pick another name: there is no other ...p function in the server.
>
> Although there are some defines
>
>> Although I prefer the p suffix myself (coming from a Lisp background),
>> I still think that we should try to keep with the conventions in the server.
>>
>> Suggestions: shall_ignore_server_id()
>>
>
> there is no problem with taking this name instead.
OK
>>> + my_sprintf(ignore_server_ids_buf,
>>> + (ignore_server_ids_buf, "%u",
> mi->ignore_server_ids.elements));
>>> + for (ulong i= 0, cur_len= strlen(ignore_server_ids_buf);
>> Suggestion, set cur_len from the result of the my_sprintf() call instead
>> of calling strlen() and re-compute the length as follows:
>>
>> cur_len= my_sprintf(ignore_server_ids_buf,
>> (ignore_server_ids_buf, "%u",
>> mi->ignore_server_ids.elements));
>>
>
> sure.
OK
[snip]
>> Is the list in the file guaranteed to be in sorted order? The reading
>> might produce a list in non-sorted order otherwise.
>>
>
> it is sorted, because the mi->ignore_server_ids is sorted.
OK
>
>>> +
>>> static bool check_io_slave_killed(THD *thd, Master_info *mi, const char
> *info)
>>> {
>>> if (io_slave_killed(thd, mi))
>>> @@ -907,7 +996,7 @@ static int get_master_version_and_clock(
>>> (master_res= mysql_store_result(mysql)))
>>> {
>>> if ((master_row= mysql_fetch_row(master_res)) &&
>>> - (::server_id == strtoul(master_row[1], 0, 10)) &&
>>> + (::server_id == (mi->master_id= strtoul(master_row[1], 0, 10)))
> &&
>> Any specific reason to restrict it to base 10 instead of just relying on
>> the 0 base?
>>
>
> Why should we rely on anything implicit if there is a clear simple way to
> demand what we need explicitly?
To be able to read integers even if they are stored in other "legal"
formats?
Never mind, this is a minor point.
[snip]
>>> + if (cur_len + slen + 4 > FN_REFLEN)
>>> + {
>>> + /*
>>> + break the loop whenever remained space could not fit
>>> + ellipses on the next cycle
>>> + */
>>> + my_sprintf(buff + cur_len, (buff + cur_len, "..."));
>> Show master info is not only for providing human-readable output, it is
>> also used by applications to get information. You need to ensure that
>> *all* information is printed *and* that it is printed in a format that
>> an application can use (it seems like you're using a space-separated
>> list here).
>>
>
> Please consider two things.
>
> 1. it will never happen in the real world that the length of the
> ignored sever list exceed FN_REFLEN/11 > 46 elements.
>
> 2. The point why I made ellipses is because conceptually we would need to
> report something instead of nothing if an accident happened (not in
> the real world).
>
> An application needs to take care of ellipsis, and I don't see any
> problem in that. I am not adding ellipesis to an existing parameter's
> format, it's a new parameter.
> I hope you share my opinion that we need merely document that (without
> refering to the real word time time :-).
OK. I would prefer some clearer way of seeing that the information is
partial, but this will do.
[snip]
>>> + s_id= uint4korr(buf + SERVER_ID_OFFSET);
>>> + if ((s_id == ::server_id && !mi->rli.replicate_same_server_id)
> ||
>>> + /*
>>> + the following conjunction deals with IGNORE_SERVER_IDS, if set
>>> + */
>>> + (mi->ignore_server_ids.elements > 0 &&
>>> + mi->repl_server_id_ignore_p(s_id) &&
>>> + /* everything is filtered out from non-master */
>>> + (s_id != mi->master_id ||
>> OK, but could you write out the comments. It was not immediate that the
>> code was correct.
>
> What do you mean "It was not immediate the code is not correct"?
> How comments can prevent that?
> And there are comments.
>
>> (I.e., that if the master is on the ignore list, all
>> events are ignored except those relating to the relay log management.)
>>
>
> Sorry, I seem to misunderstand your concern...
>
> In the immediate ignored master case the slave needs meta info.
> Do you really think I need to prove that?
No, but explain that it is the case. Just say "The following conjunction
deals with IGNORE_SERVER_IDS, if set. If the master is on the ignore
list, we still need to execute format description log events and rotate
events."
Just my few cents,
Mats Kindahl
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
