Il giorno ven 9 ago 2019 alle ore 16:45 Zili Chen <wander4...@gmail.com> 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 <eolive...@gmail.com> 于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 <h...@apache.org> ha scritto:
> >
> > > hi tison, that sounds good to me, thanks
> > >
> > > On Thu, Aug 8, 2019 at 6:40 PM Zili Chen <wander4...@gmail.com> 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 <wander4...@gmail.com> 于2019年8月8日周四 上午9:20写道：
> > > >
> > > > > @Michael
> > > > >
> > > > > FYI, it is possible by properly setting and updating suppressing
> > rules.
> > > > >
> > > > > Best,
> > > > > tison.
> > > > >
> > > > >
> > > > > Michael Han <h...@apache.org> 于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 <wander4...@gmail.com>
> > > 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 <eolive...@gmail.com> 于2019年8月6日周二 下午1:13写道：
> > > > >> >
> > > > >> > > Il giorno mar 6 ago 2019 alle ore 03:51 Zili Chen <
> > > > >> wander4...@gmail.com>
> > > > >> > > 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 <wander4...@gmail.com> 于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 <eolive...@gmail.com> 于2019年8月5日周一
> > 下午9:37写道：
> > > > >> > > > >
> > > > >> > > > >> Il giorno lun 5 ago 2019 alle ore 14:03 Zili Chen <
> > > > >> > > wander4...@gmail.com
> > > > >> > > > >
> > > > >> > > > >> 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
> > > > >> > > > >> >
> > > > >> > > > >>
> > > > >> > > > >
> > > > >> > > >
> > > > >> > >
> > > > >> >
> > > > >>
> > > > >
> > > >
> > >
> >
>
