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 > >> >
