I committed Henning's patch for LOG4J2-343. Thanks Henning for doing the work!
Ralph, I looked at the patch for LOG4J2-338 and I believe the only impact will be on the TLSSyslogAppender class. On Thu, Aug 15, 2013 at 4:08 PM, Ralph Goers <[email protected]>wrote: > No, I have not started working on LOG4J2-338. Go ahead and I will find a > way to work around the problems that will occur with that patch. > > Ralhp > > On Aug 14, 2013, at 7:41 PM, Remko Popma wrote: > > I have attached an update to Henning's patch to LOG4J2-343. > The update was necessary to resolve the conflicts caused by the LOG4J2-353 > refactoring. > > I would like to avoid doing this again so I would like to commit this > patch soon (in a few hours or so). > Ralph, have you already started working on LOG4J2-338? If not I would > like to commit this first. > > Nick, if you are concerned that this patch affects the appender.db > package, the only change to that package is this: > OLD: > public abstract class AbstractDatabaseAppender<T extends > AbstractDatabaseManager> extends AbstractAppender<LogEvent> { > NEW: > public abstract class AbstractDatabaseAppender<T extends > AbstractDatabaseManager> extends AbstractAppender { > > I really don't want to wait a few days and keep having to resolve > conflicts, > but it would also not be fair to ask others to hold off on starting new > work that may cause conflicts... > > Does that make sense? > > Remko > > > > On Thu, Aug 15, 2013 at 10:56 AM, Remko Popma <[email protected]>wrote: > >> Henning, >> >> I reviewed your patch and did not see any additional type casts. >> I think this is a misunderstanding that happened when two separate things >> were discussed together. >> >> 1. Does the Layout interface need to be generic to avoid type casts? >> 2. Does the Appender interface need to be generic to support a generic >> Layout interface (without introducing type casts)? >> >> The discussion on (1) is still ongoing, but regardless of the conclusion, >> your patch proves that the answer to (2) is no: there is no need for the >> Appender interface to be generic. >> Appender can return a generic Layout<? extends Serializable> without >> needing to be generic itself. >> >> And that is great because Appender is used *everywhere*. >> >> Remko >> >> >> >> On Thu, Aug 15, 2013 at 10:43 AM, Henning Schmiedehausen < >> [email protected]> wrote: >> >>> Gary, I do not see your point here. >>> >>> In your example, SerializedLayout would have a concrete type for the >>> toSerializable(LogEvent) method and XMLLayout would have another. >>> >>> So >>> >>> new SerializedLayout().toSerializable(logEvent) returns a LogEvent >>> XMLLayout.createLayout().toSerializable(logEvent) returns a String >>> >>> You only lose the concrete type when you to not have the actual Layout >>> instance but e.g. what appender.getLayout() returns (a Layout<? extends >>> Serializable> in which case all you can assume is that >>> toSerializable(LogEvent) returns something serializable. >>> >>> I honestly do not see where you need your type cast. >>> >>> Thanks, >>> Henning >>> >>> >>> On Wed, Aug 14, 2013 at 7:37 AM, Gary Gregory <[email protected]>wrote: >>> >>>> On Wed, Aug 14, 2013 at 9:52 AM, Remko Popma <[email protected]>wrote: >>>> >>>>> Gary, >>>>> >>>>> The change only affects Appender, Layout is still generic. >>>>> Appender does not need to be generic (it can just return a Layout<? >>>>> extends Serializable> from its getLayout() method). >>>>> >>>>> This change does not introduce any type casts (afaics). >>>>> >>>> >>>> This might be academic, but it would have to introduce type casts since >>>> we have two kinds of layouts: String and LogEvent. >>>> >>>> For example: >>>> >>>> LogEvent logEvent = null; >>>> logEvent = new SerializedLayout().toSerializable(logEvent); >>>> String xmlFragment = XMLLayout.createLayout(null, null, null, >>>> null, null, null).toSerializable(logEvent); >>>> >>>> Gary >>>> >>>> >>>> >>>>> Remko >>>>> >>>>> >>>>> >>>>> On Wed, Aug 14, 2013 at 10:11 PM, Gary Gregory <[email protected] >>>>> > wrote: >>>>> >>>>>> On Wed, Aug 14, 2013 at 4:32 AM, Remko Popma >>>>>> <[email protected]>wrote: >>>>>> >>>>>>> Patch looks nice. Much fewer "raw type" compiler warnings. Seems >>>>>>> like a big improvement to me. >>>>>>> >>>>>>> A small improvement on the patch would be to remove the (now >>>>>>> unnecessary) @param <T> javadoc comments. I have that additional change >>>>>>> done in my local workspace. >>>>>>> >>>>>>> I wouldn't mind committing this but I don't want to disrupt anyone's >>>>>>> work-in-progress: the patch modifies about 125 files. >>>>>>> >>>>>>> Is everyone ok with me committing this? I'll hold off for a day or >>>>>>> two (or less if we're all ok with this). >>>>>>> >>>>>> >>>>>> Hi All: >>>>>> >>>>>> Please also consider these thought paths: >>>>>> >>>>>> - The raw compiler warnings can also be considered as a 'task not >>>>>> finished' warning. That is, I did not go all the way in pushing generics >>>>>> through the whole code base. >>>>>> >>>>>> - Consider the Appender/Layout generics separately from the other >>>>>> generics in the code. I think this is already the case based on other >>>>>> messages I've read, but I wanted to make sure 'the baby does not get >>>>>> thrown >>>>>> out with the bath water' (a goofy saying here in the US). >>>>>> >>>>>> - Weigh was is 'correct' vs. what is 'convenient' vs. compiler >>>>>> warnings. >>>>>> >>>>>> - What is the goal? For me, the goal of the generics here are >>>>>> multiple: >>>>>> -- Have the ability to write code that, for example, can configure >>>>>> and start Log4j and then manipulate it without type casts. >>>>>> -- Avoid type casts in general. >>>>>> -- Cause compile time errors instead of a runtime errors when >>>>>> extending Log4j. >>>>>> >>>>>> Gary >>>>>> >>>>>> >>>>>>> >>>>>>> Remko >>>>>>> >>>>>>> >>>>>>> On Monday, August 12, 2013, Ralph Goers wrote: >>>>>>> >>>>>>>> I've briefly glanced at your patch and it looks reasonable to me. >>>>>>>> It leaves the Layout to continue to use generics but removes the >>>>>>>> generics >>>>>>>> from the Appenders. That seems like a reasonable thing to do. >>>>>>>> >>>>>>>> Ralph >>>>>>>> >>>>>>>> On Aug 11, 2013, at 6:34 PM, Henning Schmiedehausen wrote: >>>>>>>> >>>>>>>> Ok, that makes a lot of sense. I have reworked the patch to leave >>>>>>>> these alone and put it on a different git branch. I also opened a >>>>>>>> ticket: >>>>>>>> https://issues.apache.org/jira/browse/LOG4J2-343 and attached it >>>>>>>> as a patch so that if you do not want to deal with git, you can get it >>>>>>>> from >>>>>>>> there. >>>>>>>> >>>>>>>> I added instructions on how to get the patch from git to the ticket. >>>>>>>> >>>>>>>> >>>>>>>> On Sat, Aug 10, 2013 at 4:40 PM, Nick Williams < >>>>>>>> [email protected]> wrote: >>>>>>>> >>>>>>>>> >>>>>>>>> On Aug 10, 2013, at 6:31 PM, Henning Schmiedehausen wrote: >>>>>>>>> >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> I was toying with the log4j 2 API for a new project and I stumbled >>>>>>>>> over the fact that it uses a generic for Appender<T> without actually >>>>>>>>> being >>>>>>>>> generic. The only generic part is the Layout. So as a result there is >>>>>>>>> this >>>>>>>>> weird construct of Appender<SomeSerializableType> which is actually >>>>>>>>> dictated by the layout in use. >>>>>>>>> >>>>>>>>> >>>>>>>>> I'm relatively new to the team, so I don't know much about the >>>>>>>>> reasoning behind making Appender generic, so I can't speak to that. >>>>>>>>> I'm not >>>>>>>>> personally opposed to removing these generics, but that is a HUGE >>>>>>>>> change. >>>>>>>>> >>>>>>>>> This leads to really interesting constructs such as >>>>>>>>> >>>>>>>>> public abstract class AbstractDatabaseAppender<T extends >>>>>>>>> AbstractDatabaseManager> extends AbstractAppender<LogEvent> >>>>>>>>> >>>>>>>>> >>>>>>>>> Well this is a very different case. The <LogEvent> here is about >>>>>>>>> Layout, just as you said. The <T extends AbstractDatabaseManager> is >>>>>>>>> completely unrelated to Layout and I am _not_ in favor of removing >>>>>>>>> these >>>>>>>>> generics. >>>>>>>>> >>>>>>>>> I was wondering whether this is necessary as it makes the API very >>>>>>>>> cumbersome to use and read so I removed the generic from Appender and >>>>>>>>> subsequently went through the log4j 2 code base and mostly removed >>>>>>>>> stuff >>>>>>>>> that was no longer needed once that was gone. The result is at >>>>>>>>> >>>>>>>>> https://github.com/apache/logging-log4j2/pull/1 >>>>>>>>> >>>>>>>>> >>>>>>>>> The Apache GitHub repository is just a mirror of our SVN >>>>>>>>> repository. We can't accept or use any pull requests there. You need >>>>>>>>> to >>>>>>>>> generate an SVN patch and attach it to whatever JIRA you create. (As >>>>>>>>> such, >>>>>>>>> you should close this pull request.) >>>>>>>>> >>>>>>>>> I will also file a JIRA for this. >>>>>>>>> >>>>>>>>> I know that the 2.0 release should be coming soon (being at >>>>>>>>> beta8), but I feel that making that change in the API before it is >>>>>>>>> set in >>>>>>>>> stone with 2.0 woulc be really beneficial for anyone who wants to >>>>>>>>> port code >>>>>>>>> to 2.0 / write new code. >>>>>>>>> >>>>>>>>> >>>>>>>>> I'm sure there will be plenty of discussion about this over the >>>>>>>>> next few days. >>>>>>>>> >>>>>>>>> Thanks for considering, >>>>>>>>> Henning >>>>>>>>> >>>>>>>>> >>>>>>>>> Nick >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> E-Mail: [email protected] | [email protected] >>>>>> Java Persistence with Hibernate, Second >>>>>> Edition<http://www.manning.com/bauer3/> >>>>>> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> >>>>>> Spring Batch in Action <http://www.manning.com/templier/> >>>>>> Blog: http://garygregory.wordpress.com >>>>>> Home: http://garygregory.com/ >>>>>> Tweet! http://twitter.com/GaryGregory >>>>>> >>>>> >>>>> >>>> >>>> >>>> -- >>>> E-Mail: [email protected] | [email protected] >>>> Java Persistence with Hibernate, Second >>>> Edition<http://www.manning.com/bauer3/> >>>> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> >>>> Spring Batch in Action <http://www.manning.com/templier/> >>>> Blog: http://garygregory.wordpress.com >>>> Home: http://garygregory.com/ >>>> Tweet! http://twitter.com/GaryGregory >>>> >>> >>> >> > >
