[
https://issues.apache.org/jira/browse/SOLR-12690?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16589357#comment-16589357
]
Erick Erickson commented on SOLR-12690:
---------------------------------------
I have precommit succeeding for all the source code, I removed the bits in
check-source-patterns.groovy (thanks Christine for pointing me there) that
restricted the tests to solr/contrib and solr/core, as well as the patterns
that allowed Logger and LOG as names.
Along the way I realized that my "special cases", weren't really a problem. The
validation is a little loose, but take SolrCore, which has three loggers
declared:
* log
* requestLog
* slowLog
that all works fine because the test is actually "*if slf4j is mentioned AND
there's a valid Java identifier for logging declared AND there's at least one
logger defined that adheres strictly to the pattern, pass the file".*
So the presence of the "*log*" declaration makes the check succeed and the
*requestLog* and *slowLog* are ignored. This would also succeed if those other
loggers didn't use the MethodHandles.lookup bits I think, but don't really
think it's a big enough deal to worry about.
Similarly for ZkStateReader, the top-level
*private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());*
declaration makes the file pass and the non-static declaration in the inner
class is OK:
*private final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());*
And so on. I did see one thing that I think is incorrect and is _not_ caught by
the validation in *CheckLoggingConfiguration.java*:
*LoggerFactory.getLogger(CheckLoggingConfiguration.class);*
I changed it to
*LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());*
and will check everything in if tests pass, probably this evening.
> Regularize LoggerFactory declarations
> -------------------------------------
>
> Key: SOLR-12690
> URL: https://issues.apache.org/jira/browse/SOLR-12690
> Project: Solr
> Issue Type: Improvement
> Security Level: Public(Default Security Level. Issues are Public)
> Reporter: Erick Erickson
> Assignee: Erick Erickson
> Priority: Minor
> Attachments: SOLR-12690.patch
>
>
> LoggerFactory declarations have several different forms, they should all be:
> private static final Logger log =
> LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
> * lowercase log
> * private static
> * non hard-coded class lookup.
> I'm going to regularize all of these, I think there are about 80 currently,
> we've been nibbling away at this but I'll try to do it in one go.
> [~cpoerschke] I think there's another Jira about this that I can't find just
> now, ring any bells?
> Once that's done, is there a good way to make violations of this fail
> precommit?
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
