logging-log4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ralph Goers <ralph.go...@dslextreme.com>
Subject Re: Confusing handleExceptions vs suppressExceptions vs isExceptionSuppressed
Date Sat, 20 Jul 2013 22:27:40 GMT
I'm confused.  With your message on the 18th I thought you had changed your mind and agreed
that "suppressExceptions" was a better choice than "ignoreExceptions" since it more accurately
describes what is being done.

Ralph

On Jul 20, 2013, at 2:58 PM, Nick Williams wrote:

> The conversion from "handleExceptions"/"suppressExceptions"/"isExceptionSuppressed" to
"ignoreExceptions" has been completed.
> 
> I still have some work to do to make sure these are being used/abided by consistently
and to make sure that all appenders and managers really do let exceptions propagate.
> 
> Nick
> 
> On Jul 20, 2013, at 3:57 PM, Ralph Goers wrote:
> 
>> Yeah - that seems to work.
>> 
>> Ralph
>> 
>> On Jul 20, 2013, at 1:35 PM, Nick Williams wrote:
>> 
>>> You left out the exclamation point.
>>> 
>>> If value of "junk" is specified and the defaultValue is true then (!"false".equalsIgnoreCase("junk")
&& true) returns true.
>>> 
>>> However, it should actually be (defaultValue && !"false".equalsIgnoreCase(s))
so that it short-circuits it defaultValue is false.
>>> 
>>> Nick
>>> 
>>> On Jul 20, 2013, at 3:30 PM, Ralph Goers wrote:
>>> 
>>>> Why does this not look right to me?  If a value of "junk" is specified and
the defaultValue is true then ("false".equalsIgnoreCase("junk") && true)  return false,
which is incorrect. It should just return the default value.
>>>> 
>>>> Ralph
>>>> 
>>>> On Jul 20, 2013, at 1:18 PM, Nick Williams wrote:
>>>> 
>>>>> Correction below:
>>>>> 
>>>>> On Jul 20, 2013, at 2:38 PM, Nick Williams wrote:
>>>>> 
>>>>>> Finally got back to working on this. Noticed two things:
>>>>>> 
>>>>>> 1) On some appenders, ignoreExceptions/suppressExceptions defaults
to true. On other ones it defaults to false. We should be consistent in this, and IMO it should
default to true. Does anyone have any objection to that?
>>>>>> 
>>>>>> 2) o.a.l.l.core.helpers.Booleans.parseBoolean may or may not be working
as expected, so I wanted to check with y'all. Here's the code:
>>>>>> 
>>>>>> return Strings.isEmpty(s) ? defaultValue : Boolean.parseBoolean(s);
>>>>>> 
>>>>>> Boolean.parseBoolean() doesn't have a way to specify a default value.
It automatically defaults to false if the value passed in is anything other than "true." So,
if someone passes in a non-empty value other than "true" or "false", it will always return
false even if defaultValue is true. I propose changing it to this:
>>>>>> 
>>>>>> return "true".equalsIgnoreCase(s) || defaultValue;
>>>>> 
>>>>> return "true".equalsIgnoreCase(s) || (!"false".equalsIgnoreCase(s) &&
defaultValue);
>>>>> 
>>>>>> 
>>>>>> Does anyone have any problem with that?
>>>>>> 
>>>>>> Nick
>>>>>> 
>>>>>> On Jul 18, 2013, at 8:12 PM, Gary Gregory wrote:
>>>>>> 
>>>>>>> On Jul 18, 2013, at 20:28, Paul Benedict <pbenedict@apache.org>
wrote:
>>>>>>> 
>>>>>>>> "Proper documentation" is the key phrase. :-)
>>>>>>> 
>>>>>>> Yes! :)
>>>>>>> 
>>>>>>> Gary
>>>>>>>> 
>>>>>>>> 
>>>>>>>> On Thu, Jul 18, 2013 at 7:25 PM, Nick Williams <nicholas@nicholaswilliams.net>
wrote:
>>>>>>>> Okay. Hold everything! Lol...
>>>>>>>> 
>>>>>>>> I started working on this change and then realized something.
"Suppress" means "to forcibly put an end to," "restrain," or "prevent the development, action,
or expression of." However, "ignore" means "refuse to take notice of or acknowledge; disregard
intentionally" or "fail to consider (something significant)."
>>>>>>>> 
>>>>>>>> Though not guaranteed, I can see how a user would mistake
the word "ignore" to mean that exceptions aren't even logged, while "suppress" would be more
obvious to mean they aren't thrown (but also confusing with Java suppressed exceptions as
others pointed out). I don't think it's a big deal because no matter what we do we're going
to have to properly document this field. But I just wanted to make sure everyone had thought
about this before we proceeded.
>>>>>>>> 
>>>>>>>> Nick
>>>>>>>> 
>>>>>>>> On Jul 18, 2013, at 6:58 PM, Nick Williams wrote:
>>>>>>>> 
>>>>>>>> > Okay. I'll proceed with the change.
>>>>>>>> >
>>>>>>>> > Nick
>>>>>>>> >
>>>>>>>> > On Jul 18, 2013, at 2:08 PM, Ralph Goers wrote:
>>>>>>>> >
>>>>>>>> >> First, I appreciate having the discussion before
a code change like this is made. If that had been done I probably would have vetoed it.  But
this is the ASF where everybody gets a single voice and a single vote. Although I dislike
this change and the effect it will have on my users I won't veto it if that is what the majority
wants.
>>>>>>>> >>
>>>>>>>> >> Ralph
>>>>>>>> >>
>>>>>>>> >> On Jul 18, 2013, at 11:06 AM, Nick Williams wrote:
>>>>>>>> >>
>>>>>>>> >>> Hmmm. So it sounds like we're at an impasse.
Everyone except Ralph seems to agree to renaming it ignoreExceptions, bug Ralph said the below
in opposition. Ralph, can you clarify a little? Are you objecting to just renaming the XML/JSON
attribute (which, really, is the only thing that would break your teams' configurations)?
Or are you objecting to ignoreExceptions completely? I can't tell whether you're okay with
the isExceptionSuppressed method being renamed.
>>>>>>>> >>>
>>>>>>>> >>> I would just throw it out there that, while
I understand that it would be inconvenient to have to modify all your configurations, this
IS beta software. It is not GA yet (hopefully next month or so). Anyone who uses beta software
does so with the understanding that there could be breaking changes before GA. If you didn't
want to take that risk, I respectively submit that you shouldn't have used beta software in
production. I'm not saying, "You're wrong." I'm just questioning the motives behind your opposition.
In short:
>>>>>>>> >>>
>>>>>>>> >>> If we all agree that ignoreExceptions is a better
name that suppressExceptions (inconvenience aside), now is the time to change it. We can't
after GA.
>>>>>>>> >>>
>>>>>>>> >>> Now I'm not sure how to proceed. :-P
>>>>>>>> >>>
>>>>>>>> >>> Nick
>>>>>>>> >>>
>>>>>>>> >>> On Jul 18, 2013, at 1:02 AM, Ralph Goers wrote:
>>>>>>>> >>>
>>>>>>>> >>>> I do not have a problem with renaming handleExceptions
to exceptionSuppressed.  I do have a problem with renaming supressExceptions to ignoreExceptions,
primarily because I have a bunch of teams using Log4j 2 in production and they would have
to modify their configurations when they upgrade.  Furthermore, I can't in my wildest dreams
imagine anyone getting confused over this parameter and suppressed Throwables.
>>>>>>>> >>>>
>>>>>>>> >>>> FWIW - handleExceptions means that the Appender
"handles" the exception (i.e. it is suppressed).  I don't recall why the variables don't match
- I think I might have originally exposed "handleExceptions" and found that to be ambiguous
and renamed the config param but not the internal variable.
>>>>>>>> >>>>
>>>>>>>> >>>> Ralph
>>>>>>>> >>>>
>>>>>>>> >>>> On Jul 17, 2013, at 2:42 PM, Nick Williams
wrote:
>>>>>>>> >>>>
>>>>>>>> >>>>> Appender specifies a method, isExceptionSuppressed(),
which indicates whether exceptions thrown while appending events should be suppressed (logged
instead of re-thrown).
>>>>>>>> >>>>>
>>>>>>>> >>>>> AbstractAppender implements this method
with a private handleExceptions field and a handleExceptions constructor argument. isExceptionSuppressed()
returns handleExceptions (so, supposedly, "handle exceptions" means "take care of exceptions
instead of the user having to take care of exceptions").
>>>>>>>> >>>>>
>>>>>>>> >>>>> Everybody that extends AbstractAppender
uses the same handleExceptions constructor argument. They all define a suppressExceptions
XML attribute that is assigned to the handleExceptions constructor argument in the static
plugin factory method.
>>>>>>>> >>>>>
>>>>>>>> >>>>> This is all very confusing to me. I
just realize that I have misunderstood "handleExceptions" this whole time in the database
appenders and have assumed it was the opposite of isExceptionSuppressed() / suppressExceptions
(and, thus, have written incorrect code).
>>>>>>>> >>>>>
>>>>>>>> >>>>> Does anyone have a problem with me renaming
handleExceptions to exceptionSuppressed (to match the JavaBean isExceptionSuppressed method)
to make this less confusing?
>>>>>>>> >>>>>
>>>>>>>> >>>>> Nick
>>>>>>>> >>>>> ---------------------------------------------------------------------
>>>>>>>> >>>>> To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
>>>>>>>> >>>>> For additional commands, e-mail: log4j-dev-help@logging.apache.org
>>>>>>>> >>>>>
>>>>>>>> >>>>
>>>>>>>> >>>>
>>>>>>>> >>>> ---------------------------------------------------------------------
>>>>>>>> >>>> To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
>>>>>>>> >>>> For additional commands, e-mail: log4j-dev-help@logging.apache.org
>>>>>>>> >>>>
>>>>>>>> >>>
>>>>>>>> >>>
>>>>>>>> >>> ---------------------------------------------------------------------
>>>>>>>> >>> To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
>>>>>>>> >>> For additional commands, e-mail: log4j-dev-help@logging.apache.org
>>>>>>>> >>>
>>>>>>>> >>
>>>>>>>> >>
>>>>>>>> >> ---------------------------------------------------------------------
>>>>>>>> >> To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
>>>>>>>> >> For additional commands, e-mail: log4j-dev-help@logging.apache.org
>>>>>>>> >>
>>>>>>>> >
>>>>>>>> 
>>>>>>>> 
>>>>>>>> ---------------------------------------------------------------------
>>>>>>>> To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
>>>>>>>> For additional commands, e-mail: log4j-dev-help@logging.apache.org
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>> -- 
>>>>>>>> Cheers,
>>>>>>>> Paul
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
> 


Mime
View raw message