On Jul 20, 2013, at 3:22 PM, Ralph Goers wrote: > Logback always suppresses exceptions. I am not sure about Log4j 1.x without > looking at the code but it may suppress them as well. My first reaction is to > say that suppressing exceptions should be the default but I'd want to know > which Appenders are defaulting to not suppress them.
Turns out it was just the database appenders (the ones I added a few months ago). This was an oversight on my part, and not something that someone did intentionally. > > Gary introduced the usage of Booleans.parseBoolean in revision r1501477 on > July 9. Most of the usages that he replaced were doing > > final boolean var = stringVar == null ? true : Boolean.valueOf(stringVar); > > So var would be true if no value is specified or the string is equal ignoring > case to "true". That is kind of odd because it means you have a default of > true if you specify nothing but false if you specify an invalid value. This is exactly what Booleans.parseBoolean(String, boolean) does now, and I agree that it's odd for the very reason you pointed out. That's why I'm recommending the change. > > I'm not sure what the documentation says but I'd bet that it doesn't ever say > it works as above so I agree with this change. It just says "defaults to true." That's it. > > Ralph > > > > > > On Jul 20, 2013, at 12: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; >> >> 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 >> >
