Hi, A quick update on this thread.
ZOOKEEPER-3475 a.k.a GH-1049[1] has been sent for review. It contains commits enabling checkstyle configuration on zookeeper-server module per package, as described above. Best, tison. [1] https://github.com/apache/zookeeper/pull/1049 Enrico Olivelli <[email protected]> 于2019年8月9日周五 下午11:13写道： > Il giorno ven 9 ago 2019 alle ore 16:45 Zili Chen <[email protected]> > ha > scritto: > > > Hi Enrico, > > > > It happens my original schedule on this weekend canceled so that > > I can make progress on this thread the next two days. > > > > From my side I would prefer multiple self-contained commits(see > > below) when sending pull requests. It would helps reviews and we > > can always squash and merge them. > > > > Great. > > Looking forward to your patches ! > > I am trying to help reviews as much as possible so that patches that are > ready will be committed as soon as possible and they won't need a rebase > > I hope we don't annoy to much back ports > > Thank you > > Enrico > > > > > > > For self-contained commit, I would first enable the configuration > > while suppress for all packages under zookeeper-server, and then > > remove the suppression and do the formatting job. This, as > > consequence, enables every commit to be applied individually and > > any combination should pass CI tests. > > > > Here is all of packages under zookeeper-server module. > > > > 58 org/apache/zookeeper > > 1 org/apache/zookeeper/admin > > 31 org/apache/zookeeper/cli > > 7 org/apache/zookeeper/client > > 42 org/apache/zookeeper/common > > 3 org/apache/zookeeper/jmx > > 9 org/apache/zookeeper/metrics > > 3 org/apache/zookeeper/metrics/impl > > 104 org/apache/zookeeper/server > > 15 org/apache/zookeeper/server/admin > > 13 org/apache/zookeeper/server/auth > > 19 org/apache/zookeeper/server/command > > 9 org/apache/zookeeper/server/metric > > 16 org/apache/zookeeper/server/persistence > > 108 org/apache/zookeeper/server/quorum > > 17 org/apache/zookeeper/server/quorum/auth > > 3 org/apache/zookeeper/server/quorum/flexible > > 22 org/apache/zookeeper/server/util > > 16 org/apache/zookeeper/server/watch > > 124 org/apache/zookeeper/test > > 3 org/apache/zookeeper/util > > 1 org/apache/zookeeper/version/util > > > > (reproduce by run > > > > $ find . -exec ls -dl \{\} \; | awk '{print $9}' | grep \\.java | perl > -ne > > 'if (/\/(org.*)\//) { print $1 . "\n" }' | sort | uniq -c > > > > under zookeeper-server/src/) > > > > Best, > > tison. > > > > > > Enrico Olivelli <[email protected]> 于2019年8月9日周五 下午7:56写道： > > > > > It is better that we preserve variable names, method names, class names > > > because we could fall into bad troubles in case of cherry picks. > > > > > > We can comment out the rules that would force us to break such kind of > > > 'compatibility' > > > > > > Fangmin and Micheal, you are the maintainers of two major forks, > > > do you prefer one single commit or multiple commits per group of > > packages? > > > > > > > > > Enrico > > > > > > Il ven 9 ago 2019, 05:52 Michael Han <[email protected]> ha scritto: > > > > > > > hi tison, that sounds good to me, thanks > > > > > > > > On Thu, Aug 8, 2019 at 6:40 PM Zili Chen <[email protected]> > wrote: > > > > > > > > > Hi Michael, > > > > > > > > > > After thinking about it, I think enable checkstyle configuration > > > > > is only about formatting. And I would make sure that there is no > > > > > refactor. And forked repo maintainers can rebase their work on the > > > > > new master just by manually applying patches. If the forked repo > > > > > has been quite diverged from master of apache/zookeeper, a better > > > > > way for syncing is to cherry-pick commits from apache/zookeeper and > > > > > just ignore this checkstyle stuff. > > > > > > > > > > Best, > > > > > tison. > > > > > > > > > > > > > > > Zili Chen <[email protected]> 于2019年8月8日周四 上午9:20写道： > > > > > > > > > > > @Michael > > > > > > > > > > > > FYI, it is possible by properly setting and updating suppressing > > > rules. > > > > > > > > > > > > Best, > > > > > > tison. > > > > > > > > > > > > > > > > > > Michael Han <[email protected]> 于2019年8月8日周四 上午9:12写道： > > > > > > > > > > > >> I think a good approach is doing this incrementally. Doing this > > all > > > at > > > > > >> once > > > > > >> over entire server code base will make life much harder for > those > > > who > > > > > >> maintain their own ZooKeeper forks and has internal patches, and > > the > > > > > >> change > > > > > >> itself will be impossible to review (even though most changes > are > > > > > >> mechanical, we still need review the change). > > > > > >> > > > > > >> On Mon, Aug 5, 2019 at 10:57 PM Zili Chen <[email protected] > > > > > > wrote: > > > > > >> > > > > > >> > >I see some javadoc related issues in your gist, > > > > > >> > >didn't we disable that rule ? JavadocType > > > > > >> > > > > > > >> > The failures reported are "Missing javadoc", and > > > > > >> > what we disabled is "Empty javadoc". Fair enough to > > > > > >> > disable "Missing javadoc" for now and filed into > > > > > >> > ZOOKEEPER-3469 since a empty javadoc is also a > > > > > >> > placeholder. > > > > > >> > > > > > > >> > >Are you using some automatic tool ? > > > > > >> > > > > > > >> > Yes, IntelliJ helps a lot. > > > > > >> > Best, > > > > > >> > tison. > > > > > >> > > > > > > >> > > > > > > >> > Enrico Olivelli <[email protected]> 于2019年8月6日周二 下午1:13写道： > > > > > >> > > > > > > >> > > Il giorno mar 6 ago 2019 alle ore 03:51 Zili Chen < > > > > > >> [email protected]> > > > > > >> > > ha > > > > > >> > > scritto: > > > > > >> > > > > > > > >> > > > Hi Enrico, > > > > > >> > > > > > > > > >> > > > Thanks for your participant! > > > > > >> > > > > > > > > >> > > > Running after turn on checkstyle on zookeeper-server > > > > > >> > > > it reports about 9000 checkstyle failures(see > > failures.txt[1]) > > > > > >> > > > among 596 files(see files.txt[1]). Most of them > > > > > >> > > > are related to whitespace and import. > > > > > >> > > > > > > > > >> > > > > > > > >> > > I see some javadoc related issues in your gist, > > > > > >> > > didn't we disable that rule ? JavadocType > > > > > >> > > > > > > > >> > > > > > > > >> > > > I think I can handle it with one or two whole days but > > > > > >> > > > I'm afraid that I have no spare time until next > > > weekend(08.17). > > > > > >> > > > > > > > > >> > > > > > > > >> > > Are you using some automatic tool ? > > > > > >> > > In the past few months in my company I have been working in > > > adding > > > > > >> > > checkstyle to other projects and > > > > > >> > > we used Apache NetBeans to fix up the code, I guess IntelliJ > > can > > > > do > > > > > >> the > > > > > >> > > same. > > > > > >> > > > > > > > >> > > Enrico > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > > > >> > > > Also I would prefer make it into one big PR and separate > > > > > >> > > > commits per file, i.e., resolving failures per file for > > > > > >> > > > smoothly review. > > > > > >> > > > > > > > > >> > > > Best, > > > > > >> > > > tison. > > > > > >> > > > > > > > > >> > > > [1] > > > > > >> > https://gist.github.com/TisonKun/ba69524defbf3e9d4c99eaf2de338e5a > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > Zili Chen <[email protected]> 于2019年8月6日周二 上午9:43写道： > > > > > >> > > > > > > > > >> > > > > Hi Enrico, > > > > > >> > > > > > > > > > >> > > > > Thanks for your participant! > > > > > >> > > > > > > > > > >> > > > > Running after turn on checkstyle on zookeeper-server > > > > > >> > > > > it reports about 9000 checkstyle failures(see out.txt) > > > > > >> > > > > among 596 files(see conflicts.txt). Most of them > > > > > >> > > > > are related to whitespace and import. > > > > > >> > > > > > > > > > >> > > > > I think I can handle it with one or two whole days but > > > > > >> > > > > I'm afraid that I have no spare time until next > > > > weekend(08.17). > > > > > >> > > > > > > > > > >> > > > > Also I would prefer make it into one big PR and separate > > > > > >> > > > > commits per file, i.e., resolving failures per file for > > > > > >> > > > > smoothly review. > > > > > >> > > > > > > > > > >> > > > > Best, > > > > > >> > > > > tison. > > > > > >> > > > > > > > > > >> > > > > > > > > > >> > > > > Enrico Olivelli <[email protected]> 于2019年8月5日周一 > > > 下午9:37写道： > > > > > >> > > > > > > > > > >> > > > >> Il giorno lun 5 ago 2019 alle ore 14:03 Zili Chen < > > > > > >> > > [email protected] > > > > > >> > > > > > > > > > >> > > > >> ha > > > > > >> > > > >> scritto: > > > > > >> > > > >> > > > > > >> > > > >> > Hi zookeepers, > > > > > >> > > > >> > > > > > > >> > > > >> > Recently I've sent a pr[1] on enable checkstyle > > > > configuration > > > > > >> on > > > > > >> > > > >> > zookeeper-promethus-metrics, Enrico(committer) and > > > > > >> > > Dinesh（contributor) > > > > > >> > > > >> > kindly reviewed it but we still need another look > from > > > > > >> committer > > > > > >> > as > > > > > >> > > > >> > process required. Someone of you has spare time to > > have a > > > > > look? > > > > > >> > > > >> > > > > > > >> > > > >> > > > > > >> > > > >> Tison, > > > > > >> > > > >> Norbert approved the PR and I have merged it. > > > > > >> > > > >> We have already begun this check style work. > > > > > >> > > > >> Thank you so much to you and Mao Ling for these > efforts. > > > > > >> > > > >> > > > > > >> > > > >> > > > > > >> > > > >> > > > > > > >> > > > >> > Besides, as ZOOKEEPER-3431 described, we're going to > > > enable > > > > > >> this > > > > > >> > > > >> > configuration on zookeeper-server as following. This > is > > > > > really > > > > > >> a > > > > > >> > > major > > > > > >> > > > >> > package which contains a lot of codes involved by > multi > > > > prs. > > > > > >> I'd > > > > > >> > > like > > > > > >> > > > to > > > > > >> > > > >> > hear from the community about how to proceed. > > > > > >> > > > >> > > > > > > >> > > > >> > > > > > >> > > > >> I think that if we do it quickly there will be as few > > > > troubles > > > > > as > > > > > >> > > > >> possible. > > > > > >> > > > >> I am available for review, just ping me. > > > > > >> > > > >> > > > > > >> > > > >> > > > > > >> > > > >> > > > > > >> > > > >> > > > > > > >> > > > >> > (From where I stand, enable on the whole package at > > once > > > > > >> > > > >> > could solve the problem at once, but it would > conflict > > > > quite > > > > > a > > > > > >> lot > > > > > >> > > > >> > ongoing prs. On the other hand, sending the pr > without > > > > timely > > > > > >> > > reviews > > > > > >> > > > >> > also causes a burden to rebase it on the master and > > solve > > > > > >> conflict > > > > > >> > > > >> > again and again(since it likely conflicts with a > merged > > > > pr)) > > > > > >> > > > >> > > > > > > >> > > > >> > > > > > >> > > > >> Did you check how many violations we have con > > > > zookeeper-server > > > > > ? > > > > > >> > > > >> How much big is the work ? > > > > > >> > > > >> Do you have an estimate ? > > > > > >> > > > >> > > > > > >> > > > >> We could take into consideration to have only one big > PR. > > > > > >> > > > >> The tradeoff is that it will be very difficult to > > perform a > > > > > >> review, > > > > > >> > we > > > > > >> > > > >> will > > > > > >> > > > >> need more eyes on the diff > > > > > >> > > > >> > > > > > >> > > > >> Enrico > > > > > >> > > > >> > > > > > >> > > > >> > > > > > >> > > > >> > > > > > >> > > > >> > > > > > >> > > > >> > > > > > > >> > > > >> > Best, > > > > > >> > > > >> > tison. > > > > > >> > > > >> > > > > > > >> > > > >> > [1] https://github.com/apache/zookeeper/pull/1028 > > > > > >> > > > >> > > > > > > >> > > > >> > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > > > > > > > > > > > > > > > >
