zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Zili Chen <wander4...@gmail.com>
Subject Re: [Request For Review] ZOOKEEPER-3474 a.k.a GH-1028
Date Fri, 09 Aug 2019 14:44:55 GMT
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 <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