Thanks for pointing out, Duo. Not everyone can understand the configs for checkstyles, for me, I have read the config but could not understand it... the sequence was totally from my 'experience'. So what's more important is that we should have a docs for error prone and checkstyles, which is more friendly to developers. Best Regards Allan Yang
张铎(Duo Zhang) <[email protected]> 于2019年4月4日周四 下午12:05写道： > 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 > > > >> > > > > > >
