Ahh. I wasn't aware of that. I'll add future changes at the top. Nick
On Jul 20, 2013, at 6:44 PM, Ralph Goers wrote: > Oh - I was expecting to see them at the top since that is where new changes > are generally added. > > Ralph > > On Jul 20, 2013, at 4:23 PM, Nick Williams wrote: > >> I did update changes.xml. Not sure why you don't see it, but it's there: >> >> http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/src/changes/changes.xml?r1=1505102&r2=1505209&diff_format=h >> >> Nick >> >> On Jul 20, 2013, at 6:21 PM, Ralph Goers wrote: >> >>> I didn't see an update to changes.xml. Since this is a breaking change I >>> would definitely expect to see it in the list of changes. >>> >>> 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 >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
