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

[Davi Arnaut]

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."

> "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.

>>> 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...