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 Fri, 09 Aug 2019 03:52:22 GMT
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