Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2745) WL#4630

[Ingo Strüwing]

Hi Rafal,

Rafal Somla, 17.02.2009 08:50:

...
> - Requirements are things which implementer has to address somehow in
> order to get the patch approved.
> 
> - Suggestions are things which implementer is free to ignore - the
> approval of the patch does not depend on them.
> 
> On top of that, my understanding is that "addressing an requirement" is
> not necessarily implementing it in the code. Another possibility is to
> convince the reviewer that the original solution is OK. Also, "ignoring
> a suggestion" means, for me, that the implementer does not even have to
> justify his decision (this is to save us some time - I'll not be
> offended by silently ignoring my suggestion).


Indeed. My view is different. I expect a reviewer to carefully think
through his suggestions and be very convinced that they are a change to
the better. Still the implementor is free not to follow them. But he
should say, why he doesn't. We don't want to hide information from each
other, but learn from each other. I am always sad if someone doesn't
deem me worth a reason.

If I want to add ideas, which I'm not sure about, I would not start with
"I suggest". Instead I would either talk about an idea, use conjunctives
("might", "could"), or use a question form.

In our personal relationship, I see you as the chief architect of online
backup. So your suggestions are almost requirements to me. Also you use
to come to firm conclusions faster than me. I think, this shows that you
have a better overview and understanding of the whole feature. So I feel
a difference between you suggesting something to me and me suggesting
something to you.

> 
> Ingo Strüwing wrote:
>> Hi Rafal,
>>
>> Rafal Somla, 22.12.2008 13:14:
...
>>>> +  /*
>>>> +    First read data chunk requires search of next chunk start.
>>>> +  */
>>> [12] Suggestion: move this to the end of backup_read_metadata(). So that
>>> this
>>> function can be written under pre-condition that the stream is already
>>> positioned at a start of next chunk.
>>
>>
>> Done. I did also move the other next_chunk() calls to the end of the
>> predecessor functions. That way it looks a little more like symmetry.
>>
>> However, now these functions include the search for the next chunk,
>> which I preferred to do in the function, where I handle the next object.
>> For example, the catalog reading function does now search for the meta
>> data chunk and complains if it is missing.
>>
>> Another downside is that the error from the missing chunk ends
>> mysqlbackup and the already read information is not printed any more.
>>
>> Also it costed some time to get the coverage test right as some sections
>> changed place.
>>
>> Now that I made this change to please you, can you please explain, what
>> the win is? One less string comparison per table data chunk? Or...?
>>
> 
> Ingo, if you don't see any gain from this change then why have you
> implemented it? Note that this was just my suggestion - something you
> are not obliged to follow. If you do, then I assume you agree that it is
> a good idea.


As I said above: For me a suggestion is much more than just an idea.

>   We might have many different goals when doing this job, but
> pleasing me is certainly not one of them (btw. what you have done did
> not please me - quite opposite actually).


Hm. Then I must have misunderstood your suggestion. You suggested to
move the search for the next chunk to the end of the predecessor
function. This clearly means that you think it is a good concept to do
the search at the end instead of at begin. Now that I implemented it in
all consequence, you start to dislike it?

> 
> As a reviewer, I'd like to feel free to make suggestions in hope that
> they might be useful sometimes. But I consider you, the implementer, to
> be responsible for the implementation. After all, you create the code
> and you understand it best, including all the subtleties. Reviewing your
> code I might have some ideas which can be good or wrong. Just blindly
> following these suggestions and then throwing at me questions like the
> above feels unfair. It would be more fair if you *first* ask me
> questions, if you have any, and then decide if you want to follow the
> suggestion or not.


You don't want to throw in half-baked ideas, mark them as suggestions,
and let the implementor breed on them, do you? In fact you think about
your ideas carefully before marking them suggestions (subject to human
error), right?

> 
>>>> +  /*
>>>> +    Read data chunk.
>>>> +  */
>>> [13] Perhaps add a note explaining that memory for storing the data is
>>> allocated and freed by backup stream library so that future readers of
>>> this code will not ask the question which I asked myself: "where the
>>> memory for storing the data comes from and what happens to it when it is
>>> not needed any more?".
>>
>>
>> Done, but are you serious about the application to document, what data
>> the backup stream library allocates?
>>
> 
> Hmm, I don't understand this question - can you explain/reformulate?


Well, taking malloc(3) as an example. We won't comment in the code that
the pointer returned by malloc(3) is allocated by libc and needs to be
free(3)d after use. Instead we rely on the documentation of malloc(3) to
explain the details of the interface.

Now that I use bcat_create_item() from the stream library, you expect
the application to comment about the returned values, instead of
documenting it with bcat_create_item()?

Well, it is not the same. bcat_create_item() is a callback. So I'd try
with qsort(3). It calls back a comparison function. The semantics of all
arguments and the return value is documented with qsort(3). The
implementation of a comparison function doesn't need to repeat all of this.

> 
>> ...
>>>> +  [item position (table)]= [ snap no. ! pos in snapshot's table list ]
>>> [14] I think it is the other way around - first position of the table in
>>> the
>>> snapshot, then the snapshot number.
>>
>> No. Copied verbatim from stream_v1.c.
>>
> 
> Actually, what I'm telling you is that the documentation in stream_v1.c
> is wrong (there is BUG#40587 reported about that). If you don't want to
> correct the error in your comment then perhaps you can just remove the
> incorrect quote. Especially that the format in which the coordinates are
> stored inside the backup image is irrelevant for your code - this is
> handled by backup stream library - in your code you just read the
> coordinates from the members of st_bstream_*_info structures.


I feel the need to explain what the term "catalog coordinates" means.
And I want to show, what parts the coordinates for every object type
consists of. More details are not required here. especially it is
irrelevant here, in which order the elements appear in the backup image.
It is just important that a "table" object has coordinates consisting of
a snapshot number and a position in that snapshot's table list.

Copying a piece of documentation from stream_v1.c, which contains this
information, seemed to be most easy. A better way would have been to
point at the stream library interface documentation. But I didn't find it.

Now that I made a copy, I thought it would be better to have it
consistent. If it is wrong, I'd prefer to fix it in all copies at the
same time (BUG#40587).

Alternatively I could change the wording. So that we don't have copies
any more. But then I'd re-order the elements in a logical order, not in
image order. I find it more logical to first select the snapshot and
then select the table in its list. Not to select the table in a
snapshot's table list and then select the snapshot to use.

> 
>>>> +  /*
>>>> +    Get a reference to the global object.
>>>> +    It does not fail because we checked that pos is in range.
>>>> +    Note that the array contains pointers only.
>>>> +  */
>>>> +  bup_global= *((struct st_backup_global**) dynamic_array_ptr(array,
>>>> pos));
>>> [15] Suggestion: Here and in other places, check your assumption with
>>> assert:
>>>
>>>   DBUG_ASSERT(bup_global);
>>
>>
>> Overkill IMHO, but ok. I added at a dozen places like:
>>
>>      DBUG_ASSERT(bup_global); // Never fails, pos is checked
>>
> 
> To clarify and re-iterate: I made it a suggestion exactly because it can
> be considered an overkill (if I thought it is necessary, I'd make it a
> requirement). I think it is good but if you think it is an overkill,
> then simply don't follow the suggestion.


Since I know, how strong you are about asserts, and you didn't mention,
what could go wrong so it must be very obvious, and "check your
assumption with assert" sounds more like a command than an option, I
didn't consider to contradict.

...
>>>> +  if (opt_mysqlbackup_search ||
>>>> +      opt_mysqlbackup_snapshots ||
>>>> +      opt_mysqlbackup_catalog_summary ||
>>>> +      opt_mysqlbackup_catalog_details ||
>>>> +      opt_mysqlbackup_metadata_statements ||
>>>> +      opt_mysqlbackup_metadata_extra ||
>>>> +      opt_mysqlbackup_data_chunks ||
>>>> +      opt_mysqlbackup_data_totals ||
>>>> +      (opt_mysqlbackup_summary && !(hdr->flags &
>>>> BSTREAM_FLAG_INLINE_SUMMARY)))
>>> [25] Suggestion: wouldn't it be more clear to formulate condition when
>>> we *do not* want to read the catalog?
>>
>>
>> Hehe. We want to read the catalog, if *any* of the options is present.
>> We do not want to read, if *none* is present.
>>
>> So would inverting the condition become more clear? To my great surprise
>> there are indeed humans who find double negations easier to understand.
>> But I fail to do so.
>>
> 
> Right, the condition is such that negating it will not make it simpler.
> 
> I'm sorry to see the irony again - I asked a question in hope that it
> might help to improve the code. Sometimes negating the condition can
> simplify it a lot and often such possibility can be overlooked (I'm sure
> it happened to me). I'll take a mental note that you are not interested
> in such remarks and suggestions.


Well, I'd be interested in such remarks and suggestions if they are
either thought through, ideally with a coding suggestion, or if they are
clearly marked as being not elaborated, e.g. "Sometimes negating a
condition can simplify it a lot. Please consider if this applies here too."

...

Regards
Ingo
-- 
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Dr. Roland Bömer
Vorsitzender des Aufsichtsrates: Martin Häring   HRB München 161028