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