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