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
