Apologies. I did mention that I wanted to commit sooner (see my email 22 hours ago that starts with "I have attached an update..." and ends with "Does that make sense?". I assumed everyone had seen that message. Again sorry.
On Thu, Aug 15, 2013 at 11:55 PM, Gary Gregory <[email protected]>wrote: > On Thu, Aug 15, 2013 at 10:47 AM, Nick Williams < > [email protected]> wrote: > >> Oh. I thought we were going to wait for me to review the patch this >> weekend and weigh in. Oh, well. >> >> N >> >> > That's the way I read the thread as well... > > G > > >> On Aug 15, 2013, at 6:09 AM, Remko Popma wrote: >> >> 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 >>>>>> >>>>> >>>>> >>>> >>> >>> >> >> > > > -- > 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 >
