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 >
