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
>>>
>>
>>
>
