You are viewing a plain text version of this content. The canonical link for it is here.
Posted to log4j-dev@logging.apache.org by Remko Popma <re...@gmail.com> on 2016/05/04 15:59:22 UTC
The LOG4J2-1347 branch
I reviewed the LOG4J2-1347 branch and I like the work Mikael did here.
Replacing
the deserialize(serialize(logevent)) sequences with
Log4jLogEvent.createMemento() looks cleaner.
I could not see any issue and I don't mind if these changes are merged into
master.
(Implementing the feature requested in LOG4J2-1347 is a separate thing and
will require more work.)
Remko
Re: The LOG4J2-1347 branch
Posted by Remko Popma <re...@gmail.com>.
FYI, the relevant commit email is titled "[1/7] logging-log4j2 git commit:
LogEvent memento".
On Thu, May 5, 2016 at 2:08 AM, Remko Popma <re...@gmail.com> wrote:
> This change essentially moves all duplicate logic that called
> Log4jLogEvent.deserialize(Log4jLogEvent.serialize(event)) into a single
> place.
> There should not be any change in behaviour.
> (Unless I overlooked something.)
>
> On Thu, May 5, 2016 at 2:05 AM, Gary Gregory <ga...@gmail.com>
> wrote:
>
>> Cool. Any idea how that affects speed or memory? Does it matter?
>>
>> Gary
>> On May 4, 2016 8:59 AM, "Remko Popma" <re...@gmail.com> wrote:
>>
>>> I reviewed the LOG4J2-1347 branch and I like the work Mikael did here. Replacing
>>> the deserialize(serialize(logevent)) sequences with
>>> Log4jLogEvent.createMemento() looks cleaner.
>>>
>>> I could not see any issue and I don't mind if these changes are merged
>>> into master.
>>>
>>> (Implementing the feature requested in LOG4J2-1347 is a separate thing
>>> and will require more work.)
>>>
>>> Remko
>>>
>>
>
Re: The LOG4J2-1347 branch
Posted by Remko Popma <re...@gmail.com>.
This change essentially moves all duplicate logic that called
Log4jLogEvent.deserialize(Log4jLogEvent.serialize(event)) into a single
place.
There should not be any change in behaviour.
(Unless I overlooked something.)
On Thu, May 5, 2016 at 2:05 AM, Gary Gregory <ga...@gmail.com> wrote:
> Cool. Any idea how that affects speed or memory? Does it matter?
>
> Gary
> On May 4, 2016 8:59 AM, "Remko Popma" <re...@gmail.com> wrote:
>
>> I reviewed the LOG4J2-1347 branch and I like the work Mikael did here. Replacing
>> the deserialize(serialize(logevent)) sequences with
>> Log4jLogEvent.createMemento() looks cleaner.
>>
>> I could not see any issue and I don't mind if these changes are merged
>> into master.
>>
>> (Implementing the feature requested in LOG4J2-1347 is a separate thing
>> and will require more work.)
>>
>> Remko
>>
>
Re: The LOG4J2-1347 branch
Posted by Gary Gregory <ga...@gmail.com>.
Cool. Any idea how that affects speed or memory? Does it matter?
Gary
On May 4, 2016 8:59 AM, "Remko Popma" <re...@gmail.com> wrote:
> I reviewed the LOG4J2-1347 branch and I like the work Mikael did here. Replacing
> the deserialize(serialize(logevent)) sequences with
> Log4jLogEvent.createMemento() looks cleaner.
>
> I could not see any issue and I don't mind if these changes are merged
> into master.
>
> (Implementing the feature requested in LOG4J2-1347 is a separate thing and
> will require more work.)
>
> Remko
>
Re: The LOG4J2-1347 branch
Posted by Remko Popma <re...@gmail.com>.
Thanks, looks good.
On Thu, May 5, 2016 at 1:50 AM, Mikael Ståldal <mi...@magine.com>
wrote:
> OK, I reverted that and merged to master.
>
> On Wed, May 4, 2016 at 6:40 PM, Remko Popma <re...@gmail.com> wrote:
>
>> Well, no, not sure about that one.
>> I actually like that createMemento() under the hood calls
>> deserialize(serialize()) and uses LogEventProxy.
>>
>> I would need to do more analysis to see if using new
>> Log4jLogEvent.Builder(this) would give the exact same result but I
>> suspect there are differences.
>>
>>
>> On Thu, May 5, 2016 at 1:09 AM, Mikael Ståldal <mikael.staldal@magine.com
>> > wrote:
>>
>>> I just pushed another commit to that branch which implements
>>> MutableLogEvent.createMemento() without deserialize(serialize(event)).
>>> Does it looks good too?
>>>
>>> On Wed, May 4, 2016 at 5:59 PM, Remko Popma <re...@gmail.com>
>>> wrote:
>>>
>>>> I reviewed the LOG4J2-1347 branch and I like the work Mikael did here. Replacing
>>>> the deserialize(serialize(logevent)) sequences with
>>>> Log4jLogEvent.createMemento() looks cleaner.
>>>>
>>>> I could not see any issue and I don't mind if these changes are merged
>>>> into master.
>>>>
>>>> (Implementing the feature requested in LOG4J2-1347 is a separate thing
>>>> and will require more work.)
>>>>
>>>> Remko
>>>>
>>>
>>>
>>>
>>> --
>>> [image: MagineTV]
>>>
>>> *Mikael Ståldal*
>>> Senior software developer
>>>
>>> *Magine TV*
>>> mikael.staldal@magine.com
>>> Grev Turegatan 3 | 114 46 Stockholm, Sweden | www.magine.com
>>>
>>> Privileged and/or Confidential Information may be contained in this
>>> message. If you are not the addressee indicated in this message
>>> (or responsible for delivery of the message to such a person), you may
>>> not copy or deliver this message to anyone. In such case,
>>> you should destroy this message and kindly notify the sender by reply
>>> email.
>>>
>>
>>
>
>
> --
> [image: MagineTV]
>
> *Mikael Ståldal*
> Senior software developer
>
> *Magine TV*
> mikael.staldal@magine.com
> Grev Turegatan 3 | 114 46 Stockholm, Sweden | www.magine.com
>
> Privileged and/or Confidential Information may be contained in this
> message. If you are not the addressee indicated in this message
> (or responsible for delivery of the message to such a person), you may not
> copy or deliver this message to anyone. In such case,
> you should destroy this message and kindly notify the sender by reply
> email.
>
Re: The LOG4J2-1347 branch
Posted by Mikael Ståldal <mi...@magine.com>.
OK, I reverted that and merged to master.
On Wed, May 4, 2016 at 6:40 PM, Remko Popma <re...@gmail.com> wrote:
> Well, no, not sure about that one.
> I actually like that createMemento() under the hood calls
> deserialize(serialize()) and uses LogEventProxy.
>
> I would need to do more analysis to see if using new
> Log4jLogEvent.Builder(this) would give the exact same result but I
> suspect there are differences.
>
>
> On Thu, May 5, 2016 at 1:09 AM, Mikael Ståldal <mi...@magine.com>
> wrote:
>
>> I just pushed another commit to that branch which implements
>> MutableLogEvent.createMemento() without deserialize(serialize(event)).
>> Does it looks good too?
>>
>> On Wed, May 4, 2016 at 5:59 PM, Remko Popma <re...@gmail.com>
>> wrote:
>>
>>> I reviewed the LOG4J2-1347 branch and I like the work Mikael did here. Replacing
>>> the deserialize(serialize(logevent)) sequences with
>>> Log4jLogEvent.createMemento() looks cleaner.
>>>
>>> I could not see any issue and I don't mind if these changes are merged
>>> into master.
>>>
>>> (Implementing the feature requested in LOG4J2-1347 is a separate thing
>>> and will require more work.)
>>>
>>> Remko
>>>
>>
>>
>>
>> --
>> [image: MagineTV]
>>
>> *Mikael Ståldal*
>> Senior software developer
>>
>> *Magine TV*
>> mikael.staldal@magine.com
>> Grev Turegatan 3 | 114 46 Stockholm, Sweden | www.magine.com
>>
>> Privileged and/or Confidential Information may be contained in this
>> message. If you are not the addressee indicated in this message
>> (or responsible for delivery of the message to such a person), you may
>> not copy or deliver this message to anyone. In such case,
>> you should destroy this message and kindly notify the sender by reply
>> email.
>>
>
>
--
[image: MagineTV]
*Mikael Ståldal*
Senior software developer
*Magine TV*
mikael.staldal@magine.com
Grev Turegatan 3 | 114 46 Stockholm, Sweden | www.magine.com
Privileged and/or Confidential Information may be contained in this
message. If you are not the addressee indicated in this message
(or responsible for delivery of the message to such a person), you may not
copy or deliver this message to anyone. In such case,
you should destroy this message and kindly notify the sender by reply
email.
Re: The LOG4J2-1347 branch
Posted by Remko Popma <re...@gmail.com>.
Well, no, not sure about that one.
I actually like that createMemento() under the hood calls
deserialize(serialize()) and uses LogEventProxy.
I would need to do more analysis to see if using new
Log4jLogEvent.Builder(this) would give the exact same result but I suspect
there are differences.
On Thu, May 5, 2016 at 1:09 AM, Mikael Ståldal <mi...@magine.com>
wrote:
> I just pushed another commit to that branch which implements
> MutableLogEvent.createMemento() without deserialize(serialize(event)).
> Does it looks good too?
>
> On Wed, May 4, 2016 at 5:59 PM, Remko Popma <re...@gmail.com> wrote:
>
>> I reviewed the LOG4J2-1347 branch and I like the work Mikael did here. Replacing
>> the deserialize(serialize(logevent)) sequences with
>> Log4jLogEvent.createMemento() looks cleaner.
>>
>> I could not see any issue and I don't mind if these changes are merged
>> into master.
>>
>> (Implementing the feature requested in LOG4J2-1347 is a separate thing
>> and will require more work.)
>>
>> Remko
>>
>
>
>
> --
> [image: MagineTV]
>
> *Mikael Ståldal*
> Senior software developer
>
> *Magine TV*
> mikael.staldal@magine.com
> Grev Turegatan 3 | 114 46 Stockholm, Sweden | www.magine.com
>
> Privileged and/or Confidential Information may be contained in this
> message. If you are not the addressee indicated in this message
> (or responsible for delivery of the message to such a person), you may not
> copy or deliver this message to anyone. In such case,
> you should destroy this message and kindly notify the sender by reply
> email.
>
Re: The LOG4J2-1347 branch
Posted by Mikael Ståldal <mi...@magine.com>.
I just pushed another commit to that branch which implements MutableLogEvent
.createMemento() without deserialize(serialize(event)). Does it looks good
too?
On Wed, May 4, 2016 at 5:59 PM, Remko Popma <re...@gmail.com> wrote:
> I reviewed the LOG4J2-1347 branch and I like the work Mikael did here. Replacing
> the deserialize(serialize(logevent)) sequences with
> Log4jLogEvent.createMemento() looks cleaner.
>
> I could not see any issue and I don't mind if these changes are merged
> into master.
>
> (Implementing the feature requested in LOG4J2-1347 is a separate thing and
> will require more work.)
>
> Remko
>
--
[image: MagineTV]
*Mikael Ståldal*
Senior software developer
*Magine TV*
mikael.staldal@magine.com
Grev Turegatan 3 | 114 46 Stockholm, Sweden | www.magine.com
Privileged and/or Confidential Information may be contained in this
message. If you are not the addressee indicated in this message
(or responsible for delivery of the message to such a person), you may not
copy or deliver this message to anyone. In such case,
you should destroy this message and kindly notify the sender by reply
email.