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 Tue, 06 Aug 2019 01:50:51 GMT
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 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.

[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