[
https://issues.apache.org/jira/browse/HBASE-20478?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16450016#comment-16450016
]
Sean Busbey commented on HBASE-20478:
-------------------------------------
First some context.
The {{hbaseanti}} test has always been done as a test on the patchfile itself.
That means it doesn't know anything about the repository layout before nor
after the patch is applied. Personally, I think that makes output confusing to
understand.
here's an example result file for v2:
{code}
Use of Hadoop classification annotations in patchfile
------
66:+import org.apache.hadoop.classification.InterfaceAudience;
-------
Use of Jackson 1 classes/annotations in patchfile
------
93:+import org.codehaus.jackson.databind.MapperFeature;
94:+import org.codehaus.jackson.databind.ObjectMapper;
-------
Use of commons-logging instead of slf4j in patchfile
------
121:+import org.apache.commons.logging.Log;
122:+import org.apache.commons.logging.LogFactory;
-------
{code}
v2 added line numbers, because just having some lines that were pulled out of
the patch file was confusing. Even _with_ these line numbers I suspect it's
still confusing that they're referring to lines _in the patchfile_, since most
committers aren't going to open up the patch file itself. For contributors
attempting to use qabot to self service getting their patch ready prior to a
committer showing up, this seems even more likely to confuse.
More over, the current approach is a pretty big sledgehammer. It equally flags
patches that _remove_ these offenses as well as those where they just show up
in the contextual part of the patch (things that are neither added nor being
removed by the change).
If you look at the output for the WIP patch I'll be posting here shortly, it'll
be even more confusing because there will be results that flag on the patch to
{{dev-support/hbase-personality.sh}} for implementing these checks. This
happens despite the fact that we know we only ever need to check source files
for these things.
Maybe it's time to redo these things as a comparison of the source tree before
and after the patch is applied. It'll probably take me another hour or two to
do that. But it means we'd get errors that specify that a particular file is
making use of e.g. commons-logging instead of just that it happens at all. It
also means we could run the check in nightly (tests that look at the patchfile
itself don't run in our nightly checks because there is no patch under
consideration).
What do y'all think?
> precommit check for "hbase antipatterns" should log details and add a footer
> as needed
> --------------------------------------------------------------------------------------
>
> Key: HBASE-20478
> URL: https://issues.apache.org/jira/browse/HBASE-20478
> Project: HBase
> Issue Type: Improvement
> Components: test
> Reporter: Sean Busbey
> Assignee: Sean Busbey
> Priority: Major
> Attachments: HBASE-20478.0.patch, HBASE-20478.1.patch,
> HBASE-20478.2.patch, HBASE-20478.WIP.patch
>
>
> came up in discussion on HBASE-20332. our check of "don't do this" things in
> the codebase doesn't log the specifics of complaints anywhere, which forces
> those who want to follow up to reverse engineer the check.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
