Hi Rafal,
sorry for quoting your full email. I believe you said that you prefer it
this way.
Rafal Somla, 10.04.2008 12:07:
> Ingo,
>
> Thanks for finding time to look at my patch and for your valuable
> comments. I'll prepare an updated patch for you to have a final look. In
> the meantime see my replies below.
>
> Ingo Strüwing wrote:
>> Hi Rafal,
>>
>> rsomla@stripped, 31.03.2008 13:28:
>> ...
>>> ChangeSet@stripped, 2008-03-31 13:28:32+02:00, rafal@quant.(none) +6 -0
>>> BUG#34721 (Backup driver can't refuse to provide driver):
>>> This patch modifies backup kernel so that it falls back to built
>>> in backup engines in case a native backup engine can not be
>>> created. This is for the special case when storage engine defines
>>> a get_backup_engine() factory function (inside the handlerton), but
>>> that function fails to create a backup engine instance. Previous
>>> code haven't dealt correctly with that case. Now it puts a warning
>>> and tries the built-in engines.
>>
>> Formally a changeset comment should say 1. what was the problem, and 2.
>> how was it fixed. The latter is well explained. So I'd appreciate if you
>> prepend a sentence saying what the problem was. The bug synopsis is too
>> short IMHO.
>>
>
> OK, comment changed to:
>
> BUG#34721 (Backup driver can't refuse to provide driver):
>
> This patch modifies backup kernel so that it falls back to built in backup
> engines in case a native backup engine can not be created.
>
> If storage engine defines a get_backup_engine() factory function (inside
> the
> handlerton), but that function fails to create a backup engine then the
> current
> code interrupts backup/restore operation and reports error. This is not the
> expected behaviour. This patch fixes the backup engine selection logic
> so that
> in the above case it puts warning and continues using the built-in engines.
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.
>
>
>> ...
>>> sql/backup/backup_info.cc@stripped, 2008-03-31 13:28:29+02:00,
>>> rafal@quant.(none) +44 -9
>>> Debug code injected into has_native_backup() function which sets
>>> MyISAM backup factory function to a dummy one. This is used to
>>> test the backup engine selection
>>> logic.
>>> In find_backup_engine: in case a native backup engine has
>>> been selected for a given table, check if created Native_snapshot
>>> object is valid (in particular, has successfully created the
>>> engine). Only then use it for that table, otherwise try other
>>> backup engines including the built-in ones.
>>
>> Great comment. However I would prefer to have the explanation, how it
>> works, in a code comment. IMHO changeset file comments do just need to
>> say _what_ was changed, which helps when resolving merge conflicts.
>>
>
> I inserted this comment in the code as well. However, I'm not sure how a
> perfect cset comment should look in this case so I'm leaving it as it is
> (assuming that giving too much information never hurts).
Ok.
>
>>> sql/backup/be_native.h@stripped, 2008-03-31 13:28:29+02:00,
>>> rafal@quant.(none) +1 -1
>>> If the get_backup_engine() factory function defined by a storage
>>> engine failes
>>
>> Typo: failes -> fails
>>
>>> to create a native backup engine don't treat it as a (fatal)
>>> error. In that case we will try to use default, built-in backup
>>> engines. Thus only warning is printed.
>>
>> Again, here I would like to see something like "Changed
>> ER_BACKUP_CREATE_BE to report as a warning". And the above comment moved
>> into the code.
>>
>
> I moved comment into the code and changed cset comment to:
>
> In Native_snapshot::init() report ER_BACKUP_CREATE_BE as warning, not as
> error.
Ok.
>
>> ...
>>> +++ b/mysql-test/t/backup_no_be-master.opt 2008-03-31 13:28:29 +02:00
>>> @@ -0,0 +1 @@
>>> +--debug=d,backup_test_dummy_be_factory
>>
>> To try to get rid of a server restart and thus this option file, can't
>> we remember the old value of hton->get_backup_engine and restore it
>> after doing the related checks?
>>
>
> I was thinking about that and concluded that it is not easy to
> implement. But after your suggestions I will give it another try. You
> should see the outcome in the next patch I'll send.
>
>> ...
>>> +++ b/sql/backup/backup_info.cc 2008-03-31 13:28:29 +02:00
>> ...
>>> /// 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.
>
>>> {
>>> handlerton *hton= se_hton(se);
>>>
>>> + /*
>>> + The code below is used to test the backup engine selection logic.
>> ...
>>> + dummy backup engine factory which never creates any engines.
>>> + */
>>
>> Please remove the trailing space from the */ line and indent it
>> correctly (one space less).
>>
>
> 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'll be grateful if in the future we skip wasting our time on fixing
> code spacing issues...
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.
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?
>
>>> + DBUG_EXECUTE_IF("backup_test_dummy_be_factory", + if (hton ==
>>> myisam_hton) hton->get_backup_engine= dummy_backup_engine_factory;);
>>
>> If you find (or create) some temporary storage in 'se', you could save
>> the current value of hton->get_backup_engine so that you can restore it
>> later. Then we could avoid a server restart.
>>
>>> +
>>> return hton && hton->get_backup_engine;
>>> }
>>>
>>> @@ -83,16 +117,17 @@ Backup_info::find_backup_engine(const ba
>>> {
>>> Native_snapshot *nsnap= new Native_snapshot(m_ctx, se);
>>> DBUG_ASSERT(nsnap);
>>
>> Not part of your patch, but as it pops up in the changeset: This is ok
>> in a prototype. But before push to main the assert must be changed to
>> error handling.
>>
>
> Good point. I removed the assertion and added checks for nsnap != NULL.
> If nsnap == NULL (we couldn't create Native_snapshot object) the code
> will proceed to trying the built-in backup engines. If built-in engines
> don't work then an error will be reported.
>
>> ...
>>> + 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?
>
>>> + else
>>> + delete nsnap;
>>> }
>>
>> Here you could restore the myisam get_backup_engine pointer if you saved
>> it above.
>>
>
> Right, it might work. I'll just move the error inserted code to the same
> function.
>
>> ...
>>
>> Otherwise approved if you agree with my above change requests and
>> considered my suggestions.
>>
>
> I'll send you a new patch so that you can verify your approval. I'll
> also uncheck the review box on the bug page so we can better trace the
> status.
>
> Rafal
>
>
I'll look at that patch soon.
Regards
Ingo
--
Ingo Strüwing, Senior Software Developer
MySQL GmbH, Dachauer Str. 37, D-80335 München
Geschäftsführer: Kaj Arnö - HRB München 162140
