Oh. I thought we were going to wait for me to review the patch this weekend and weigh in. Oh, well.
N 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 >> JUnit in Action, Second Edition >> Spring Batch in Action >> 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 >> JUnit in Action, Second Edition >> Spring Batch in Action >> Blog: http://garygregory.wordpress.com >> Home: http://garygregory.com/ >> Tweet! http://twitter.com/GaryGregory >> >> >> > >
