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 <[email protected]> wrote:
>>>>>>>
>>>>>>>> "Proper documentation" is the key phrase. :-)
>>>>>>>
>>>>>>> Yes! :)
>>>>>>>
>>>>>>> Gary
>>>>>>>>
>>>>>>>>
>>>>>>>> On Thu, Jul 18, 2013 at 7:25 PM, Nick Williams
>>>>>>>> <[email protected]> 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: [email protected]
>>>>>>>> >>>>> For additional commands, e-mail:
>>>>>>>> >>>>> [email protected]
>>>>>>>> >>>>>
>>>>>>>> >>>>
>>>>>>>> >>>>
>>>>>>>> >>>> ---------------------------------------------------------------------
>>>>>>>> >>>> To unsubscribe, e-mail: [email protected]
>>>>>>>> >>>> For additional commands, e-mail: [email protected]
>>>>>>>> >>>>
>>>>>>>> >>>
>>>>>>>> >>>
>>>>>>>> >>> ---------------------------------------------------------------------
>>>>>>>> >>> To unsubscribe, e-mail: [email protected]
>>>>>>>> >>> For additional commands, e-mail: [email protected]
>>>>>>>> >>>
>>>>>>>> >>
>>>>>>>> >>
>>>>>>>> >> ---------------------------------------------------------------------
>>>>>>>> >> To unsubscribe, e-mail: [email protected]
>>>>>>>> >> For additional commands, e-mail: [email protected]
>>>>>>>> >>
>>>>>>>> >
>>>>>>>>
>>>>>>>>
>>>>>>>> ---------------------------------------------------------------------
>>>>>>>> To unsubscribe, e-mail: [email protected]
>>>>>>>> For additional commands, e-mail: [email protected]
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Cheers,
>>>>>>>> Paul
>>>>>>
>>>>>
>>>>
>>>
>>
>
