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 Sat, 24 Aug 2019 00:06:47 GMT
FYI ZOOKEEPER-3517[1][2].

We still have one invocation of the checkstyle plugin per module.

The strategy is that we turn on the strict one at project level while
the simple one at contrib module level as an override. This is an invert
from what we previously did.

Best,
tison.

[1] https://issues.apache.org/jira/browse/ZOOKEEPER-3517
[2] https://github.com/apache/zookeeper/pull/1060


Enrico Olivelli <eolivelli@gmail.com> 于2019年8月18日周日 下午1:13写道:

> Il dom 18 ago 2019, 03:26 Zili Chen <wander4096@gmail.com> ha scritto:
>
> > Of course I volunteer to do.
> >
>
> Great
>
> >
> > To clarify the process, we want to merge checkstyle-strict.xml into
> > checkstyle.xml and thus turn on rules in current checkstyle-strict.xml
> > on project level, while also removing configurations in plugins field
> > of pom.xml in submodules. Is this process expected?
> >
>
> Yes
>
> >
>
> We don't need to apply checkstyle-strict.xml to 'contrib' it is another big
> work that is it not worth to do, as such Java modules are not maintained.
>
> It would be awesome to have only one invocation of the checkstyle plugin
> per module.
>
> Maybe we can just call the simple checkstyle.xml from the parent module of
> recipes and of contrib, but I did not try
>
>
> Enrico
>
>
>
>
> > Best,
> > tison.
> >
> >
> > Enrico Olivelli <eolivelli@gmail.com> 于2019年8月18日周日 上午3:36写道:
> >
> > > We have merged this useful work.
> > >
> > > Now we have to merge the two checkstyle files/plugin executions into
> only
> > > one.
> > >
> > > We have one project wide configuration (all projects, contrib...) that
> > > check for '@author' and the new one that is a full checkstyle run.
> > >
> > > Tison, do you want to complete the work?
> > >
> > > Thank you
> > >
> > > Enrico
> > >
> > >
> > >
> > > Il dom 11 ago 2019, 13:05 Zili Chen <wander4096@gmail.com> ha scritto:
> > >
> > > > 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 <eolivelli@gmail.com> 于2019年8月9日周五 下午11:13写道:
> > > >
> > > > > 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