zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Han <h...@apache.org>
Subject Re: [Request For Review] ZOOKEEPER-3474 a.k.a GH-1028
Date Thu, 08 Aug 2019 01:12:33 GMT
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