Hi Ingo,
Looks that we differ in our interpretation of coding guidelines and in
understanding of how they should be used. Let's try to differ in a beautiful
way, as they say :) I have changed the patch a bit in an attempt to satisfy some
of your concerns and at the same time be faithful to my beliefs... - see below.
Ingo Strüwing wrote:
>
> I would use past tense for the problem description and try to make it
> somewhat more high-level:
>
> The problem was that a storage engine specific backup driver could not
> refuse to do the backup if it could/would/wanted not to do it. The whole
> backup process stopped in this case.
>
> The intended behavior it to use a built-in backup engine for that
> storage engine is such cases.
>
> This patch modifies the backup kernel so that it falls back to the
> built-in backup engines in case a native backup engine can not be created.
>
> In detail each storage engine defines a get_backup_engine() factory
> function (inside the handlerton). If that function fails to create a
> backup engine then the backup engine selection logic does now push a
> warning and continues using a built-in backup engine for this storage
> engine.
>
I changed patch description to:
The problem was that a storage engine could not refuse to create a native backup
engine without stopping the whole backup process with an error. The intended
behaviour is to use a built-in backup engine for that storage engine is such
case.
This patch modifies the backup kernel so that it falls back to the
built-in backup engines in case a native backup engine can not be created.
In detail each storage engine defines a get_backup_engine() factory
function (inside the handlerton). If that function fails to create a
backup engine then the backup engine selection logic does now push a
warning and continues using a built-in backup engine for this storage
engine.
Thanks for your help in better formulating it.
>>> ...
>>>> /// Determine if a given storage engine has native backup support.
>>>> static
>>>> bool has_native_backup(storage_engine_ref se)
>>> Please fix the format of the function comment.
>>>
>> Hmm, what's wrong with the above one - it is a valid doxygen comment.
>> Having a full-fledged documentation for this function would be an
>> overkill. The one line description I gave should be enough to understand
>> the purpose of the function.
>
> It should indeed be enough to understand the purpose of the function.
> But I checked the coding guidelines again and don't see an exception
> that would catch this case.
>
> Shall we really start a new discussion about pros and cons of common
> coding guidelines?
>
> Unless you change the guidelines or convince me that the above adheres
> to them, I say it is a violation.
>
I could try to argue that using an example as a normative requirement is not
correct and that what is not forbidden is allowed (you don't need an explicit
exception in the rules for that) but I really think such discussions are ridiculous.
As it happens, the guidelines contain this clause:
Every structure, class, method or function should have a description unless it
is very short and its purpose is obvious.
I will use it and remove the comment altogether, removing the controversy at the
same time. Clearly the function is very short and its purpose is obvious.
>> Sorry Ingo, but I am alergic to such review comments. Can you please
>> point me at a coding guideline which *requires* me to remove the spece
>> before '*/'? The only relevant rule I can see is:
>>
>> "When writing multi-line comments please put the '/*' and '*/' on their
>> own lines, put the '*/' below the '/*', put a line break and a two-space
>> indent after the '/*', do not use additional asterisks on the left of
>> the comment"
>>
>> And I clearly put '*/' on its own line and below '/*' as required, and
>> satisfied all the other requirements.
>
> You have:
> /*
> */
> which is not exactly below in the sense of the guideline. The examples
> in the guideline show them exactly below. And almost all comments I saw
> thus far are like the examples in the guideline.
>
I changed the spacing so that '*/' is *exactly* below '/*', even though our
coding guidelines require it only to be below, not exactly below.
> Imagine, another one would be grateful if we skip discussing comments,
> another one would be grateful if we skip discussing DBUG_ASSERTs,
> another one would be grateful if we skip discussing naming conventions,
> another one would be grateful if we skip discussing performance, ...
>
> "Indentation and Spacing" is a big chapter in the guidelines. For me
> this is important stuff. Badly indented or spaced code slows me down in
> reading.
>
> Admittedly, one space in a comment is not a real issue. But refraining
> from pointing at it forever doesn't seem right to me. Remember, I did
> not require another look at your patch. I just said it is wrong, but
> approved it in its whole. If you decided to ignore my comment then it
> would have gone unnoticed and you would just bear the responsibility for
> it. But you cannot demand me to knowingly ignore something I deem wrong.
>
Ok, so we have learned where we differ :)
> A different story would be if you have a different coding style for the
> backup code, like InnoDB and Falcon have their own style. Do you?
>
No, we try to follow the common coding guidelines, but perhaps we understand
them differently...
>>>> + if (nsnap->is_valid())
>>>> + {
>>>> + snapshots.push_front(nsnap);
>>>> + native_snapshots.insert(se, nsnap);
>>>> +
>>>> + if (nsnap->accept(tbl, se)) + snap= nsnap;
>>>> + }
>>> Shouldn't you delete nsnap if accept() returns FALSE? You do it only if
>>> is_valid() returns FALSE.
>>>
>> No, the valid native snapshot object is kept to be potentially used for
>> other tables. It is deleted when Backup_info object is destroyed.
>
> Hm. But where is a reference to the nsnap object stored that can survive
> the end of the embracing block? And why do you delete nsnap when
> is_valid() fails?
>
If the snapshot is valid, it is added to the snapshots list (a member of
Backup_info class). This is where the pointer is stored and in Backup_info
destructor all snapshots added to the list are deleted:
Backup_info::~Backup_info()
{
using namespace backup;
close();
// delete Snapshot_info instances.
Snapshot_info *snap;
List_iterator<Snapshot_info> it(snapshots);
while ((snap= it++))
delete snap;
}
Rafal
