List:Commits« Previous MessageNext Message »
From:Rafal Somla Date:June 27 2007 9:59am
Subject:Re: bk commit into 5.2 tree (rafal:1.2524)
View as plain text  
Chuck,

Thank you for the review - I know it is a lot of code to go through! I have 
implemented most of your suggestions but I still object your recommendation to 
define constants for -X return codes - see my comments below.

I'm preparing patch with these changes and some other fixes. I'll send it to you 
when it is ready.

Best,
Rafal

Chuck Bell wrote:
>  
> Rafal,
> 
> This is a tough one to call. Since I could not apply the patches, I used
> your repository that you made available to me. Unfortunately, it does not
> compile on Windows generating 69 errors and 39 warnings.
> 
> So I have to say this patch is not ok and we should work together to get the
> compile and patch problems sorted out before this patch is pushed. It also
> makes me concerned for the other 3 patches but fortunately they are much
> smaller.
> 
> I will also try compiling the code on Linux tomorrow. I am not concerned so
> much for the warnings, but some of the errors I am seeing leads me to think
> there may be code or class-level design changes needed to correct them.
> 
> Compiler Errors on Windows
> --------------------------
> C2593 - operator == is ambiguous 
> C3861 - identifier not found (can be corrected with EXTRA_DEBUG declaration)
> C2065 - 'SQLCOM_BACKUP' identifier not found (and SQLCOM_RESTORE ...)
> C2051 - case expression not constant
> 
> Compiler Warnings on Windows
> ----------------------------
> C4291 - no matching operator found
> C4355 - "this" used in base member initializer list
> C4099 - type name first seen using 'struct'
> C4065 - switch contains default but no case labels
>

Let's try to resolve these problems together - I need you expertise in windows 
building environment. I'm trying to get access to our replication windows server 
but I'm not sure if I manage and if it will be possible to compile there 
(ideally share a desktop).

If this doesn't work we can do it over skype and or IRC: you will compile and 
show me the errors and I'll look at the code and try to figure out what needs to 
be changed.

> Concerns about the code in this patch
> -------------------------------------
> Comments need spell checking. "paralled" "lenght" 
> 

I run the spell checker again - only found the "lenght" misspelling.

> There are a couple of places that have commented out code. If you really
> want this code to stay where it is, please place a comment block around it
> and explain why it is there else please remove them from the code.
> 
> I think it would be best for our 5.2 baseline to remove all of the "old"
> code from these patches. Please remove them from the 5.2 patches, but
> perhaps leave them in the prototype repository. I think the code would be
> clearer without the extra methods.
> 

Good point. I removed all old code - it is stored in the prototype tree.

> I still don't like the use of ordinals for return codes to trigger modes or
> switches (return -7 means nothing to me and the use of -7 isn't documented
> well). I strongly recommend using enums or const instead. Besides making the
> code more readable, the names of the enums or consts would also help
> comprehension considerably.
>

I hope you read my explanations for this design in LLD of WL#3904 (section 
"Return values").

To give an example - suppose you call mysql_backup() and it returns -3. What 
does that mean? Primarily, it means "An error happened during execution and it 
was reported via backup logger". That's it - any other non-zero value has the 
same meaning.

This is the main purpose of the return value which has only these two meanings: 
error or no-error. But, to add flexibility, there is an "unofficial" extension 
of this idea. Namely, functions return different non-zero values from different 
places where errors were detected. If for some reason a caller needs to know 
where the error happened inside mysql_backup() it can consult function code and 
see that -3 is returned after failed backup of table data, to distinguish it 
from two other places where errors are detected. Now, if the same value -3 is 
returned from e.g. write_table_data() it still means "an error has happened" but 
now it indicates different place which is inside this function (it is error when 
adding backup pump to the scheduler).

If we want to define constants for all these return codes we would have as many 
of them as there are functions reporting errors times the number of places 
inside one function where errors are detected. This is a huge number and a lot 
of work to enter all these constants (I'd even have problem with inventing names 
for them). Then there is extra work in maintaining the constants when code 
changes. And what we get in return? I claim that almost nothing. I don't think 
we add to the clarity of the code with these heaps of constants with not-so-well 
defined meaning. Besides, as I explain in WL we *don't want* to precisely 
describe error which has happened with the return value - the only information 
we want to pass is error/non-error distinction. Anything beyond that (i.e. 
different non-zero values) should be considered an "undocumented" extension 
which can be safely ignored by people who don't know about it.

> Questions (for Chuck's curiosity)
> ---------------------------------
> What is the <pre> tag for?
> 

@pre and @post (this is how they should be written in doxygen, I think) mean 
"pre-condition" and "post-condition" for a function.

> What should backup::BUSY be used for and when? I think some places I use
> "PROCESSING" when I may need to use "BUSY."
> 

The two replies have the same effect that get/send_data() call will be repeated. 
The difference is in telling the kernel if the data in the buffer is used by 
driver or not.

- BUSY reply means that driver can't process current get/send_data() call and 
thus the data in the buffer is not used - kernel is free to reuse buffer (its 
memory are) for other purposes if it likes,

- PROCESSING reply means that driver started to process the get/send_data() 
request but is not finished yet. Kernel will call get/send_data() again to check 
when the request is completed. In each of these calls *the same buffer* will be 
passed as driver is possibly using data in the buffer (perhaps there is a 
separate thread reading/writing it). Thus the buffer is "blocked" by the driver 
and can't be reused.

> General Comments 
> ----------------
> I've noticed you don't follow the same doxygen block format everywhere. Some
> places have:
> 
> /**
>   * My comment
>   * My comment
>   */
> 
> While others have:
> 
> /**
>    My comment
>    My comment
>   */ 
> 
> Which is correct? Please change all comment blocks so they use the same
> format. BTW: If the second one is the correct way, let me know so I can
> change  my code too! ;)
>

Looking at other parts of the code it seems that the second variant is being 
used (sorry...). I changed all comments in my code to that format.

> Some single line comments use // instead of /*   */.
>

Fixed.

> Please remove extra *'s from block comments that use the format:
> 
> /***********************
>   My comment
>  ***********************/
> 

Not sure what you mean by "extra *'s". I've removed stars in front of comment 
content lines.

Rafal

Thread
bk commit into 5.2 tree (rafal:1.2524)rsomla25 Jun
  • RE: bk commit into 5.2 tree (rafal:1.2524)Chuck Bell27 Jun
    • Re: bk commit into 5.2 tree (rafal:1.2524)Rafal Somla27 Jun
    • Re: bk commit into 5.2 tree (rafal:1.2524)Rafal Somla27 Jun