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 Fri, 09 Aug 2019 11:56:13 GMT
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