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

[Jon Stephens]

Davi Arnaut wrote:
> Hi Jon,
> 
> On 11/24/09 6:06 AM, Jon Stephens wrote:
>>
>> 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. :)
> 
> It also means "3. /formal/ coming after in time or order; later : a date 
> posterior to the first Reform Bill."

Maybe, but I've never heard it used thos way that I can recall.

> 
>> "Subsequent" means "later" or "following".
> 
> You are right, "subsequent" is much better.
> 
>>>>> 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".
> 
> Correct. But this might change to include everything that could be 
> written to the error log.

Then perhaps we should not use 'warnings' in the option name?

> 
>>>> 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.
> 
> Heh. You've got a point, those are indeed clearer descriptions. But 
> throughout the literature this kind of technique is referred to as "rate 
> limiting": http://en.wikipedia.org/wiki/Rate_limiting
> 
>> The words "interval" and "volume" are sufficiently descriptive IMO.
> 
> Either way is fine by me.
> 
>> 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)";
> 
> Sounds good to me.
> 
>> 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.
> 
> Now you are killing me :-)
> 
>> 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?"
> 
> If the user activated the above options, he should now those by now.
> 
>> "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!
> 
> ok, ok...
> 


-- 


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