[
https://issues.apache.org/jira/browse/LUCENE-7788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17081387#comment-17081387
]
Erick Erickson commented on LUCENE-7788:
----------------------------------------
[~cpoerschke] [~mbraun688] [~jpountz] [~dsmiley] [~varun] Spamming all of
you from several JIRAs since you've all commented. Trying to centralize here.
I looked at trying to use the current validate source patterns, but it operates
on regexes. I don't think regexes can do what we want here. And even if one
could be created, it'd be horrible to maintain.
Here's what I've got. I wrote a new groovy script we could include as a new
Gradle task (part of "check"?) that
1> flags all logger declarations that are not exactly "log". If we fix that,
then we can check for log.info() etc. blindly
2> Collects all log statements (including multi-line), strips all the quoted
text out and examines the remainder for "+" or function calls and dumps the
line out if found. This would be a failure going forward.
3> does _NOT_ do <2> if the log line has //ok or // ok etc. (case insensitive).
Or maybe we want "//function calls cheap" or "//calls cheap". Hmm, thinking
about it "//calls cheap" at least gives a hint whereas "//ok" gives no clue so
I'll propose that as the tag.
<3> addresses David's comments elsewhere that we should leave it to the
programmer's discretion if a log message that does a function call is "cheap
enough". If we don't adopt some convention like this, then this stuff will
creep back.
So far, there are 4,325 lines flagged, and the run takes less than 3 seconds so
I think it's reasonable to add to check. NOTE: I intend to add this to the
Gradle build only. I'll clean up what I find for 8x, and if some things creep
back in over the next few months, I don't mind if we catch them later.
So first I want people's opinions on
* whether adding a tag to verified log messages that add function calls is the
way to go here. It's kind of clumsy, but it takes care of both littering the
code with a ton of "if (log.isDebugEnabled){}" clauses and allowing expensive,
unchecked log messages to creep back in the code. Ok, it litters the code with
comments I concede
* Whether adding this to check is reasonable
If there's consensus, I'll start moving this forward. WARNING: this will mean
there are a _LOT_ of code changes, I'll try to do them a couple dozen files at
a time. Once it's clean, I'll add the Gradle task if that's the consensus. I'm
sure I'll find things that aren't currently caught, but before going there I
want to be sure the work won't be wasted.
Let me know what you all think.
> fail precommit on unparameterised log.trace messages
> ----------------------------------------------------
>
> Key: LUCENE-7788
> URL: https://issues.apache.org/jira/browse/LUCENE-7788
> Project: Lucene - Core
> Issue Type: Task
> Reporter: Christine Poerschke
> Assignee: Christine Poerschke
> Priority: Minor
> Attachments: LUCENE-7788.patch, LUCENE-7788.patch
>
>
> SOLR-10415 would be removing existing unparameterised log.trace messages use
> and once that is in place then this ticket's one-line change would be for
> 'ant precommit' to reject any future unparameterised log.trace message use.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
