Hey Allan, your import order template is still incorrect, we do not treat
java and javax specially...
The rules in checkstyles are
<module name="ImportOrder">
<property name="groups"
value="*,org.apache.hbase.thirdparty,org.apache.hadoop.hbase.shaded"/>
<property name="option" value="top" />
<property name="ordered" value="true"/>
<property name="sortStaticImportsAlphabetically" value="true"/>
</module>
So the correct way is
import static
import all other imports
import org.apache.hbase.thirdparty.*
import org.apache.hadoop.hbase.shaded.*
Allan Yang <[email protected]> 于2019年4月4日周四 上午11:57写道：
> Import Order is quite confusing and there is no document for it. After many
> attempt, I finally figure out the right sequence and set it into my IDEA's
> auto import, now it doesn't bother me anymore.
> The right import order are:
> import static
> import java.*
> import javax.*
> import all other imports
> import org.apache.hbase.thirdparty.*
> import org.apache.hadoop.hbase.shaded.*
>
> I think we should add some docs about our error prone rules, like import
> order, and line length limitation...
> Best Regards
> Allan Yang
>
>
> Andrew Purtell <[email protected]> 于2019年4月4日周四 上午10:24写道：
>
> > Error prone used to be an optional profile and not run by precommit by
> > default. That is what I contributed.
> >
> > One of the main motivations for integrating error prone in the first
> place
> > was the idea we would use it to implement our own static checks for HBase
> > specific coding conventions and invariants. This hasn’t happened yet. It
> > might not. If this doesn’t happen the added value is marginal and we
> should
> > not run it during precommit, especially if it does not produce stable
> > results.
> >
> > > On Apr 3, 2019, at 4:25 PM, 张铎(Duo Zhang) <[email protected]>
> wrote:
> > >
> > > Please be patient sir, IIRC the error prone integration was proposed by
> > you
> > > and done by Mike Drob at the first place. The result is not stable for
> a
> > > long time, you can see HBASE-21895 and related issues, it does not
> always
> > > generate the same warnings for an unchanged file. We tried to upgrade
> the
> > > version of error prone, and also sort the javac warnings as the order
> is
> > > not stable too, but it seems that the result is still not stable
> enough.
> > > The AbstractTestDLS is not touched recently I believe.
> > >
> > > So I think we could remove the error prone check from the pre commit,
> or
> > > add it to the ignore list first, where it will generate a -0, not a -1,
> > > just like the tests4tests check. Just a one line change in our
> > personality
> > > script. And maybe open an issue for the error prone project to see
> > whether
> > > the guys there know how to deal with these problems.
> > >
> > > And for the import order, I’m using eclipse too, I can share you the
> > import
> > > order settings. And yeah it has been customized by me so I’m not sure
> > > whether the one in the dev-support is still valid. We should keep it in
> > > sync so we do not confuse developers.
> > >
> > > Thanks.
> > >
> > > Andrew Purtell <[email protected]>于2019年4月4日 周四05:15写道：
> > >
> > >> Regarding the error-prone issues I am talking about, here is one
> > >>
> > >>
> > >>
> >
> https://builds.apache.org/job/PreCommit-HBASE-Build/16578/artifact/patchprocess/diff-compile-javac-root.txt
> > >>
> > >> [WARNING]
> > >>
> >
> /testptch/hbase/hbase-server/src/test/java/org/apache/hadoop/hbase/master/AbstractTestDLS.java:[654,54]
> > >> [UnusedVariable] The parameter 'master' is never read.
> > >>
> > >> The submitted patch does not touch this file AbstractTestDLS and this
> > javac
> > >> warning has the format of an error-prone finding.
> > >>
> > >>
> > >>> On Wed, Apr 3, 2019 at 2:11 PM Andrew Purtell <[email protected]>
> > wrote:
> > >>>
> > >>> I use Eclipse. Eclipse orders imports automatically per formatter
> > >>> settings. I have installed the HBase formatter from our dev-support
> > into
> > >>> all of the relevant workspaces. This sometimes fails to do the right
> > >> thing
> > >>> as far as checkstyle reporting in precommit is concerned. I have
> tried
> > >>> moving the indicated imports around by hand when this happens and it
> > >> still
> > >>> complains. I should not be required to use another IDE. (I won't.)
> > >> Perhaps
> > >>> the Eclipse formatter definition we ship in the project needs an
> > update.
> > >>> (From a contributor POV this shouldn't be necessary.) It's not like I
> > am
> > >>> trying to get away with being sloppy.
> > >>>
> > >>> HBASE-15560 is one.
> > >>> HBASE-22114 amplifies it by having I think some unfortunate
> > interactions
> > >>> between how Yetus decides what has changed and how to test it and
> what
> > is
> > >>> being attempted.
> > >>>
> > >>>> On Wed, Apr 3, 2019 at 1:27 PM Josh Elser <[email protected]>
> wrote:
> > >>>>
> > >>>> Yeah, can you share some evidence of what you've been running into,
> > >>>> Andrew?
> > >>>>
> > >>>> The nit-picky tools are always a pain in the rear (especially when
> > >>>> working across other branches) -- agree with you there. Can we help
> > >>>> lessen the pain by making it more clear what to run/inspect when QA
> > >>>> reports something other than a +1?
> > >>>>
> > >>>> e.g. "I see you had some checkstyle issues. Please fix them and you
> > can
> > >>>> re-verify locally by running `mvn ...`"
> > >>>>
> > >>>> I think, long run, these tools are nice to push towards consistency
> > on.
> > >>>> Wondering if there's more we can do to make working with them easier
> > >>>> before backtracking.
> > >>>>
> > >>>>> On 4/3/19 4:05 PM, Xu Cang wrote:
> > >>>>> "I think we need to revert the recent error-prone related work"
> > >>>>> -- Andrew, can you please give an example about this? Such as a
> JIRA
> > >>>> that
> > >>>>> has this kind of failure in pre-commit build.
> > >>>>>
> > >>>>> " Checkstyle's
> > >>>>> ImportOrder is one that always trips me up and no matter where I
> > place
> > >>>> the
> > >>>>> imports continues to complain."
> > >>>>>
> > >>>>> -- I had some struggles about this too, though, after I installed
> > >>>>> checkstyle plugin in intellij and used it before generating
> patches,
> > >> it
> > >>>>> became less painful. I don't have any objection removing the check
> at
> > >>>> the
> > >>>>> same time. Contributors should still try their best to organize
> > >> imports
> > >>>>> cleanly and orderly.
> > >>>>>
> > >>>>>
> > >>>>> ""
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> On Wed, Apr 3, 2019 at 12:02 PM Andrew Purtell <
> [email protected]>
> > >>>> wrote:
> > >>>>>
> > >>>>>> I have been contributing to this project for more than ten years
> and
> > >>>> have
> > >>>>>> noticed it is increasingly difficult to do so.
> > >>>>>>
> > >>>>>> For me the issues come down to precommit results. Precommit is a
> > very
> > >>>>>> useful tool, but *only if committers are attentive to fixing
> > breaking
> > >>>>>> changes immediately*. This has been an eternal problem.
> > >>>>>>
> > >>>>>> Also in fairness some problems I've thought are external to my
> patch
> > >>>> have
> > >>>>>> turned out to be indirect consequences. Here the issue is I'm not
> > >> able
> > >>>> to
> > >>>>>> trust precommit so true positive results are still sometimes
> > suspect.
> > >>>>>>
> > >>>>>> I think we need to revert the recent error-prone related work,
> this
> > >>>> seems
> > >>>>>> to be the cause of some of the false failures in precommit jobs
> I've
> > >>>> looked
> > >>>>>> at.
> > >>>>>>
> > >>>>>> In other cases we should adjust some static check settings.
> > >>>> Checkstyle's
> > >>>>>> ImportOrder is one that always trips me up and no matter where I
> > >> place
> > >>>> the
> > >>>>>> imports continues to complain. I'm at a loss and it's really a
> > >> trivial
> > >>>>>> matter. Let's just turn it off.
> > >>>>>>
> > >>>>>> The transient issues we sometimes face with Apache build infra are
> > >>>> possibly
> > >>>>>> tolerable, I'm not referring to those.
> > >>>>>>
> > >>>>>> --
> > >>>>>> Best regards,
> > >>>>>> Andrew
> > >>>>>>
> > >>>>>> Words like orphans lost among the crosstalk, meaning torn from
> > >> truth's
> > >>>>>> decrepit hands
> > >>>>>> - A23, Crosstalk
> > >>>>>>
> > >>>>>
> > >>>>
> > >>>
> > >>>
> > >>> --
> > >>> Best regards,
> > >>> Andrew
> > >>>
> > >>> Words like orphans lost among the crosstalk, meaning torn from
> truth's
> > >>> decrepit hands
> > >>> - A23, Crosstalk
> > >>>
> > >>
> > >>
> > >> --
> > >> Best regards,
> > >> Andrew
> > >>
> > >> Words like orphans lost among the crosstalk, meaning torn from truth's
> > >> decrepit hands
> > >> - A23, Crosstalk
> > >>
> >
>
