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

[Daogang Qu]

Hi Jon,
Thanks for the good comments.

Best Regards,

Daogang

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