zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Enrico Olivelli <eolive...@gmail.com>
Subject Re: [Request For Review] ZOOKEEPER-3474 a.k.a GH-1028
Date Fri, 09 Aug 2019 15:13:27 GMT
Il giorno ven 9 ago 2019 alle ore 16:45 Zili Chen <wander4096@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 <eolivelli@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 <hanm@apache.org> ha scritto:
> >
> > > hi tison, that sounds good to me, thanks
> > >
> > > On Thu, Aug 8, 2019 at 6:40 PM Zili Chen <wander4096@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 <wander4096@gmail.com> 于2019年8月8日周四 上午9:20写道:
> > > >
> > > > > @Michael
> > > > >
> > > > > FYI, it is possible by properly setting and updating suppressing
> > rules.
> > > > >
> > > > > Best,
> > > > > tison.
> > > > >
> > > > >
> > > > > Michael Han <hanm@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 <wander4096@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 <eolivelli@gmail.com> 于2019年8月6日周二
下午1:13写道:
> > > > >> >
> > > > >> > > Il giorno mar 6 ago 2019 alle ore 03:51 Zili Chen <
> > > > >> wander4096@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 <wander4096@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 <eolivelli@gmail.com>
于2019年8月5日周一
> > 下午9:37写道:
> > > > >> > > > >
> > > > >> > > > >> Il giorno lun 5 ago 2019 alle ore 14:03
Zili Chen <
> > > > >> > > wander4096@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
> > > > >> > > > >> >
> > > > >> > > > >>
> > > > >> > > > >
> > > > >> > > >
> > > > >> > >
> > > > >> >
> > > > >>
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message