Hi Ceki, Thanks for your quick follow up. My comments will be below as well. On Sun, Aug 11, 2019 at 10:26 PM Ceki Gülcü <ceki@qos.ch> wrote:
Hi Wessel,
Comments below.
On 09/08/2019 01:09, Wessel van Norel wrote:
I've not yet created the test cases for the EventRecordingLogger, I'll do that in case it's indeed unintentional that it's loosing the Throwables. I've added two tests for the changes I made to the MessageFormatter. While I was at it I also applied some improvements IntelliJ suggested.
Very good. In general, it is preferable if the more meaningful changes are separate from more cosmetic ones even if both are welcome.
Sure no problem. Do you want a separate JIRA Ticket for it?
You can see my changes on my fork:
https://github.com/qos-ch/slf4j/compare/master...delgurth:eventrecordinglogg...
I have made some comments there.
From those remarks:
Instead, an analogous check can be placed in recordEvent() avoiding repetition.
Sounds like a good plan. But I'll have to think about how to deal with the 2 object argument calls. Since if just make an two element array out of it and then have to make it into a one element array and a throwable that's a bit wasteful, isn't it?
In any case, I think the unit test as well the change to the org.slf4j.event.EventRecodingLogger.recordEvent method are warranted. I propose to discuss changes to MessageFormatter class separately.
Ok. I'll create a Jira ticket about the MessageFormatter. The stacktrace as last parameter is great for users of slf4j, but for the API implementation it's a bit of a pain. Do you have a performance benchmark around slf4j or specific the messageformatter (since the javadoc says the slf4j version is 10 times faster then the version provided by java itself)? I found a recently created SLF4J JMH project: https://github.com/wsargent/slf4j-benchmark but I'm not sure if that project is doing the proper benchmarking. Also can you please create a Jira ticket describing the bug in
EventRecodingLogger.recordEvent?
Will do.
Many thanks,
-- Ceki
You're most welcome. Kind regards, Wessel _______________________________________________
slf4j-dev mailing list slf4j-dev@qos.ch http://mailman.qos.ch/mailman/listinfo/slf4j-dev