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 Sun, 18 Aug 2019 05:13:32 GMT
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