<div dir="ltr"><div>Hi Ceki,</div><div><br></div><div>Thanks for your quick follow up. My comments will be below as well.<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Aug 11, 2019 at 10:26 PM Ceki Gülcü <<a href="mailto:ceki@qos.ch">ceki@qos.ch</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
Hi Wessel,<br>
<br>
Comments below.<br>
<br>
On 09/08/2019 01:09, Wessel van Norel wrote:<br>> I've not yet created the test cases for the EventRecordingLogger, I'll <br>
> do that in case it's indeed unintentional that it's loosing the <br>
> Throwables. I've added two tests for the changes I made to the <br>
> MessageFormatter. While I was at it I also applied some improvements <br>
> IntelliJ suggested.<br>
<br>
Very good. In general, it is preferable if the more meaningful changes <br>
are separate from more cosmetic ones even if both are welcome.<br></blockquote><div><br></div><div>Sure no problem. Do you want a separate JIRA Ticket for it? <br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> You can see my changes on my fork:<br>
> <br>
> <a href="https://github.com/qos-ch/slf4j/compare/master...delgurth:eventrecordinglogger?expand=1" rel="noreferrer" target="_blank">https://github.com/qos-ch/slf4j/compare/master...delgurth:eventrecordinglogger?expand=1</a><br>
<br>
I have made some comments there.<br></blockquote><div><br></div><div>From those remarks:</div><div><br></div><div>>  Instead, an analogous check can be placed in recordEvent() avoiding repetition.</div><div><br></div><div>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?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
In any case, I think the unit test as well the change to the <br>
org.slf4j.event.EventRecodingLogger.recordEvent method are warranted. I <br>
propose to discuss changes to MessageFormatter class separately.<br></blockquote><div> </div><div>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: <a href="https://github.com/wsargent/slf4j-benchmark">https://github.com/wsargent/slf4j-benchmark</a> but I'm not sure if that project is doing the proper benchmarking.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Also can you please create a Jira ticket describing the bug in<br>
EventRecodingLogger.recordEvent?<br></blockquote><div><br></div><div>Will do. <br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Many thanks,<br>
<br>
--<br>
Ceki<br></blockquote><div><br></div><div>You're most welcome.</div><div><br></div><div>Kind regards,</div><div>Wessel</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
_______________________________________________<br>
slf4j-dev mailing list<br>
<a href="mailto:slf4j-dev@qos.ch" target="_blank">slf4j-dev@qos.ch</a><br>
<a href="http://mailman.qos.ch/mailman/listinfo/slf4j-dev" rel="noreferrer" target="_blank">http://mailman.qos.ch/mailman/listinfo/slf4j-dev</a></blockquote></div></div>