Thanks for your detailed response. I agree. Simplicity brings value.
And I agree that performance degradation of format method vs. native concat
is negligible in the warn context.
I need to look at the trace method to see the difference between the two
calls (because I do believe that ("text" + obj) is equivalent to ("text" +
obj.toString()). Is this assumption wrong?)
Will run gradle precommit locally to stop being annoyed by these details.
Ilan
Le mer. 20 mai 2020 à 20:49, Erick Erickson <erickerick...@gmail.com> a
écrit :
> Ilan:
>
> Technically it may be true (although frankly I don’t know for sure), but
> practically at warn level any extra work isn’t worth the confusion.
>
> What do I mean by that? Well, we built up a huge debt with all sorts of
> debug and info and even trace messages that either concatenated strings or
> called methods or… See LUCENE-7788 for the amount of code that had to be
> changed.
>
> So when I wrote the checker, I decided to flag all the concatenations in
> the code reasoning that for warn level messages, any extra work was so
> trivial that you couldn’t really measure it overall, and it’d be worth it
> to keep from “broken window" issues. I.e. someone sees log.warn(“whatever”
> + “whenever”) and thinks it’s OK to use that pattern for debug, trace or
> info.
>
> I don’t particularly care if warn messages are a little inefficient on the
> theory that there should be so few of them that it’s not measurable.
>
> All that said, this is absolutely a judgement call that I made trying to
> balance the technical with the practical. Given the number of people who
> contribute to the code, I think it’s worthwhile to keep certain patterns
> out of the code. Especially given how obscure logging costs are. The
> difference between 'log.trace(“message {}”, object.toString())’ and
> 'log.trace(“message {}”, object)’ for instance is unknown to a _lot_ of
> developers. Including me before I started looking at logging in general ;)
>
> Best,
> Erick
>
> > On May 20, 2020, at 1:05 PM, Ilan Ginzburg <ilans...@gmail.com> wrote:
> >
> > This might have been discussed previously but since I'm seeing this
> behavior...
> >
> > Gradle precommit check does not allow code such as:
> > log.warn("Only in tree one: " + t1);
> >
> > And forces changing it into:
> > log.warn("Only in tree one: {}", t1);
> >
> > I do understand such constraints for debug level logs to not pay the
> concatenation cost if the log is not output (unless the log is inside an if
> block testing debug level), but for logs generally turned on (which I
> assume warn logs are), this is counter productive: the second form is less
> efficient during execution than the first one.
> >
> > Ilan
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
> For additional commands, e-mail: dev-h...@lucene.apache.org
>
>
