Re: bzr commit into mysql-5.1-bugteam branch (Dao-Gang.Qu:3192) Bug#42851

[Jon Stephens]

Hi!

Comments inline. Note that I've given you a bit more then you asked 
for... :)

Daogang Qu wrote:
> Davi Arnaut wrote:
>> Hi Dao-Gang,
>>
>> A few comments below.
>>
>> On 11/23/09 6:42 AM, Dao-Gang.Qu@stripped wrote:
>>> #At file:///home/daogangqu/mysql/bzrwork/bug42851/mysql-5.1-bugteam/ 
>>> based on revid:davi.arnaut@stripped
>>>
>>>   3192 Dao-Gang.Qu@stripped    2009-11-23
>>>        Bug #42851      Spurious "Statement is not safe to log in 
>>> statement format." warnings
>>>
>>>        Warnings in error log make error log grow too large.
>>>
>>>        The problem can be resolved by limiting the rate of messages 
>>> that are
>>>        written to the log. A volume of messages that is less than or 
>>> equal to
>>>        the specified rate is written to the log, whereas the volume 
>>> of messages
>>>        that exceeds the rate is discarded.
>>>
>>>        For example,
>>>
>>>        log-warnings-ratelimit-interval = 10
>>>        log-warnings-ratelimit-burst = 5
>>>
>>>        This allows 5 log messages per 10 seconds. The sixth (and 
>>> posterior)

s/posterior/subsequent/

"Posterior" means "the back of something" or "the end of something". 
It's also a synonym for for the place on the human body that makes 
contact with the chair when we sit down. :)

"Subsequent" means "later" or "following".

>>>        attempts to write a log message within a 10 seconds interval are
>>>        discarded.
>>
>> [..]
>>
>>> +
>>> +void Log_to_file_event_handler::rate_limit_exceeded()
>>> +{
>>> +  sql_print_error("log-warnings-ratelimit-burst parameter's value (N) \
>>> +exceeded; discarding the rest of error messages");
>>
>> Please, break the string in a way that preserves indentation:
>>
>> sql_print_errror("log-warnings-ratelimit-burst parameter's value (N) "
>>                  "exceeded; discarding the rest of error messages");
>>
>> and the message looks like broken english. 

This message looks okay to me, but for an error message, I'd rather 
discard the "'s".

Also, these options suppress warnings, but not errors, correct? SO the 
message to should refer to "warnings", not "error messages".

>> What is N? 

I would assume that it's the value the user has set for 
log-warnings-ratelimit-burst, and this is what I would expect to be shown.

Also, I think the option names themselves are not very good. They're too 
long, "ratelimit" isn't a word, and "rate-limit" is basically noise; so 
I *strongly* recommend that these two options be renamed as follows:

log-warnings-interval (for log-warnings-ratelimit-interval)
log-warnings-volume (for log-warning-ratelimit-burst)

If I were reviewing this patch, I would *insist* that the options be 
renamed before I'd approve it... but since I am just a humble tech 
writer who doesn't actually get to tell anybody what to do, I only get 
to make annoying suggestions.

The words "interval" and "volume" are sufficiently descriptive IMO.

So we'd wind up with something like this

sql_print_error("log-warnings-volume (%s) exceeded; skipping additional"
                 "warnings for this log-warnings-interval (%s seconds)";

The %s placeholders should be replaced by the current values of 
log-warnings-interval and log-warnings-limit, respectively. If it's not 
possible to retrieve these values and insert them into the message, then 
drop the "(%s)" and "(%s seconds)" but don't stick a literal "(N)" in 
there -- that's just confusing.

This might seem a little long, but it tells the user exactly what's 
going on.

>> Please, consult 
>> the documentation team if you think the original messages weren't 
>> clear enough -- which i think were good enough, if a user does not 
>> understand it, he should look it up in the docs where he/she will find 
>> a extended explanation.

Well, you've asked this member of the docs team, and this member of the 
docs team thinks that warning and error messages should actually tell 
the user what's going on. :)

> Hi Jon,
> What's your idea about the description?
> 
> Davi still like the original info as following:
> sql_print_error("Burst exceeded, rate limiting.");

I don't like this "message" at all -- it's highly 
broken/ambiguous/non-descriptive.

All it does is to invite questions: "What's a 'burst'? Exceeding it in 
what way?"; "'Rate limiting'? Do you mean 'limiting rate'? What rate? 
Limited how?"

"RTFM" should *never* be used as an excuse for poor design choices, 
especially when it comes to crucial info such as error messages and 
warnings. For shame!

My 2 öre,

jon.

> 
> Best Regards,
> 
> Daogang
>>
>>> +}
>>> +
>>> +void Log_to_file_event_handler::rate_limit_reset(unsigned int 
>>> suppressed)
>>> +{
>>> +  sql_print_error("Rate limit lifted, %u warning messages were 
>>> suppressed.",
>>> +                  suppressed);
>>>   }
>>>
>>>   void Log_to_file_event_handler::init_pthread_objects()
>>>
>>> === modified file 'sql/log.h'
>>> --- a/sql/log.h    2009-06-18 13:52:46 +0000
>>> +++ b/sql/log.h    2009-11-23 08:42:40 +0000
>>> @@ -16,6 +16,9 @@
>>>   #ifndef LOG_H
>>>   #define LOG_H
>>>
>>> +extern ulong log_warnings_ratelimit_interval;
>>> +extern ulong log_warnings_ratelimit_burst;
>>> +
>>>   class Relay_log_info;
>>>
>>>   class Format_description_log_event;
>>> @@ -459,15 +462,88 @@ public:
>>>   };
>>>
>>>
>>> +/**
>>> +  Rate limiter.
>>> +*/
>>> +
>>> +class Rate_limit
>>> +{
>>> +  public:
>>> +    Rate_limit() : m_count(0), m_suppressed(0), m_begin(0),
>>> +                   interval(0), burst(0)
>>> +    {}
>>
>> Please add a virtual destructor.
>>
> 
> 


-- 


Jon Stephens - jon.stephens@stripped
Technical Writer
MySQL Documentation Team
Sun Microsystems AB
MySQL and Software Infrastructure Group
Liljeholmen (Stockholm), Sweden
Summer: UTC +02.00 / Winter: UTC +01.00
Mobile: [phone]
Skype: plastic-fish
MySQL: www.mysql.com
Sun: www.sun.com