You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by steven lu <lu...@gmail.com> on 2022/09/25 01:09:24 UTC
[DISCUSS] BP-58 Change the API for org.apache.bookkeeper.stats.Counter
Hi BookKeepers, I've changed the API for org.apache.bookkeeper.stats.Counter:
BP-58 <https://github.com/apache/bookkeeper/issues/3502>
"Motivation"
The latency of the OpStatsLogger.registerSuccessfulEvent calculation is to
convert the time to milliseconds
but Counter.add nothing to do
so when using Counter for latency statistics, the time unit and
OpStatsLogger are not unified, which is easy to be misleading.
then we unified latency metric unit
"Proposal"
We already have the Counter metric for counting , we just need to refactor
it and expose it as a public method.
My idea is to change the API of org.apache.bookkeeper.stats.Counter
when using Counter for count metric, use Counter.addCount,intead of old
API(Counter.add)
when using Counter for latency metric, use Counter.addLatency
"Release"
because it change the API,so we don't cherry pick this into released
branches
so it should go into a major release, like Release-4.16, then we can update
its implementation in other projects, like pulsar
What do you think about it?
Re: [DISCUSS] BP-58 Change the API for org.apache.bookkeeper.stats.Counter
Posted by steven lu <lu...@gmail.com>.
Got it
thanks Nicolò Boschi
Nicolò Boschi <bo...@gmail.com> 于2022年9月27日周二 16:10写道：
> +1 for the BP, thanks Steven
>
> Usually is a good practice to put the pull request in the "Draft" status to
> ensure nobody is able to merge it
>
> Nicolò Boschi
>
>
> Il giorno mar 27 set 2022 alle ore 08:59 Enrico Olivelli <
> eolivelli@gmail.com> ha scritto:
>
> > Hello,
> > The PR has been merged without running a formal VOTE.
> >
> > https://github.com/apache/bookkeeper/pull/3501
> >
> > I think that given the impact of this change for this time we can move
> > forward and consider the BP "accepted"
> >
> > A few people already commented by "supporting" the BP
> >
> > Thoughts ?
> >
> > Enrico
> >
> > Il giorno mar 27 set 2022 alle ore 03:13 Yong Zhang
> > <zh...@gmail.com> ha scritto:
> > >
> > > LGTM.
> > >
> > > Best,
> > > Yong
> > >
> > > On Mon, 26 Sept 2022 at 15:56, wenbing shen <ol...@gmail.com>
> > > wrote:
> > >
> > > > +1 （non-binding）
> > > >
> > > > Thanks.
> > > > Wenbing Shen
> > > >
> > > > ZhangJian He <sh...@gmail.com> 于2022年9月26日周一 11:15写道：
> > > >
> > > > > +1 （non-binding）
> > > > >
> > > > > Thanks
> > > > > ZhangJian He
> > > > >
> > > > > On Sun, 25 Sept 2022 at 20:45, Enrico Olivelli <
> eolivelli@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > I agree
> > > > > >
> > > > > > Enrico
> > > > > >
> > > > > > Il Dom 25 Set 2022, 03:09 steven lu <lu...@gmail.com> ha
> > > > scritto:
> > > > > >
> > > > > > > Hi BookKeepers, I've changed the API for
> > > > > > > org.apache.bookkeeper.stats.Counter:
> > > > > > > BP-58 <https://github.com/apache/bookkeeper/issues/3502>
> > > > > > >
> > > > > > >
> > > > > > > "Motivation"
> > > > > > >
> > > > > > > The latency of the OpStatsLogger.registerSuccessfulEvent
> > calculation
> > > > is
> > > > > > to
> > > > > > > convert the time to milliseconds
> > > > > > > but Counter.add nothing to do
> > > > > > > so when using Counter for latency statistics, the time unit and
> > > > > > > OpStatsLogger are not unified, which is easy to be misleading.
> > > > > > > then we unified latency metric unit
> > > > > > >
> > > > > > > "Proposal"
> > > > > > >
> > > > > > > We already have the Counter metric for counting , we just need
> to
> > > > > > refactor
> > > > > > > it and expose it as a public method.
> > > > > > > My idea is to change the API of
> > org.apache.bookkeeper.stats.Counter
> > > > > > >
> > > > > > > when using Counter for count metric, use
> Counter.addCount,intead
> > of
> > > > old
> > > > > > > API(Counter.add)
> > > > > > > when using Counter for latency metric, use Counter.addLatency
> > > > > > >
> > > > > > > "Release"
> > > > > > >
> > > > > > > because it change the API,so we don't cherry pick this into
> > released
> > > > > > > branches
> > > > > > > so it should go into a major release, like Release-4.16, then
> we
> > can
> > > > > > update
> > > > > > > its implementation in other projects, like pulsar
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > What do you think about it?
> > > > > > >
> > > > > >
> > > > >
> > > >
> >
>
Re: [DISCUSS] BP-58 Change the API for org.apache.bookkeeper.stats.Counter
Posted by Nicolò Boschi <bo...@gmail.com>.
+1 for the BP, thanks Steven
Usually is a good practice to put the pull request in the "Draft" status to
ensure nobody is able to merge it
Nicolò Boschi
Il giorno mar 27 set 2022 alle ore 08:59 Enrico Olivelli <
eolivelli@gmail.com> ha scritto:
> Hello,
> The PR has been merged without running a formal VOTE.
>
> https://github.com/apache/bookkeeper/pull/3501
>
> I think that given the impact of this change for this time we can move
> forward and consider the BP "accepted"
>
> A few people already commented by "supporting" the BP
>
> Thoughts ?
>
> Enrico
>
> Il giorno mar 27 set 2022 alle ore 03:13 Yong Zhang
> <zh...@gmail.com> ha scritto:
> >
> > LGTM.
> >
> > Best,
> > Yong
> >
> > On Mon, 26 Sept 2022 at 15:56, wenbing shen <ol...@gmail.com>
> > wrote:
> >
> > > +1 （non-binding）
> > >
> > > Thanks.
> > > Wenbing Shen
> > >
> > > ZhangJian He <sh...@gmail.com> 于2022年9月26日周一 11:15写道：
> > >
> > > > +1 （non-binding）
> > > >
> > > > Thanks
> > > > ZhangJian He
> > > >
> > > > On Sun, 25 Sept 2022 at 20:45, Enrico Olivelli <eo...@gmail.com>
> > > > wrote:
> > > >
> > > > > I agree
> > > > >
> > > > > Enrico
> > > > >
> > > > > Il Dom 25 Set 2022, 03:09 steven lu <lu...@gmail.com> ha
> > > scritto:
> > > > >
> > > > > > Hi BookKeepers, I've changed the API for
> > > > > > org.apache.bookkeeper.stats.Counter:
> > > > > > BP-58 <https://github.com/apache/bookkeeper/issues/3502>
> > > > > >
> > > > > >
> > > > > > "Motivation"
> > > > > >
> > > > > > The latency of the OpStatsLogger.registerSuccessfulEvent
> calculation
> > > is
> > > > > to
> > > > > > convert the time to milliseconds
> > > > > > but Counter.add nothing to do
> > > > > > so when using Counter for latency statistics, the time unit and
> > > > > > OpStatsLogger are not unified, which is easy to be misleading.
> > > > > > then we unified latency metric unit
> > > > > >
> > > > > > "Proposal"
> > > > > >
> > > > > > We already have the Counter metric for counting , we just need to
> > > > > refactor
> > > > > > it and expose it as a public method.
> > > > > > My idea is to change the API of
> org.apache.bookkeeper.stats.Counter
> > > > > >
> > > > > > when using Counter for count metric, use Counter.addCount,intead
> of
> > > old
> > > > > > API(Counter.add)
> > > > > > when using Counter for latency metric, use Counter.addLatency
> > > > > >
> > > > > > "Release"
> > > > > >
> > > > > > because it change the API,so we don't cherry pick this into
> released
> > > > > > branches
> > > > > > so it should go into a major release, like Release-4.16, then we
> can
> > > > > update
> > > > > > its implementation in other projects, like pulsar
> > > > > >
> > > > > >
> > > > > >
> > > > > > What do you think about it?
> > > > > >
> > > > >
> > > >
> > >
>
Re: [DISCUSS] BP-58 Change the API for org.apache.bookkeeper.stats.Counter
Posted by Enrico Olivelli <eo...@gmail.com>.
Hello,
The PR has been merged without running a formal VOTE.
https://github.com/apache/bookkeeper/pull/3501
I think that given the impact of this change for this time we can move
forward and consider the BP "accepted"
A few people already commented by "supporting" the BP
Thoughts ?
Enrico
Il giorno mar 27 set 2022 alle ore 03:13 Yong Zhang
<zh...@gmail.com> ha scritto:
>
> LGTM.
>
> Best,
> Yong
>
> On Mon, 26 Sept 2022 at 15:56, wenbing shen <ol...@gmail.com>
> wrote:
>
> > +1 （non-binding）
> >
> > Thanks.
> > Wenbing Shen
> >
> > ZhangJian He <sh...@gmail.com> 于2022年9月26日周一 11:15写道：
> >
> > > +1 （non-binding）
> > >
> > > Thanks
> > > ZhangJian He
> > >
> > > On Sun, 25 Sept 2022 at 20:45, Enrico Olivelli <eo...@gmail.com>
> > > wrote:
> > >
> > > > I agree
> > > >
> > > > Enrico
> > > >
> > > > Il Dom 25 Set 2022, 03:09 steven lu <lu...@gmail.com> ha
> > scritto:
> > > >
> > > > > Hi BookKeepers, I've changed the API for
> > > > > org.apache.bookkeeper.stats.Counter:
> > > > > BP-58 <https://github.com/apache/bookkeeper/issues/3502>
> > > > >
> > > > >
> > > > > "Motivation"
> > > > >
> > > > > The latency of the OpStatsLogger.registerSuccessfulEvent calculation
> > is
> > > > to
> > > > > convert the time to milliseconds
> > > > > but Counter.add nothing to do
> > > > > so when using Counter for latency statistics, the time unit and
> > > > > OpStatsLogger are not unified, which is easy to be misleading.
> > > > > then we unified latency metric unit
> > > > >
> > > > > "Proposal"
> > > > >
> > > > > We already have the Counter metric for counting , we just need to
> > > > refactor
> > > > > it and expose it as a public method.
> > > > > My idea is to change the API of org.apache.bookkeeper.stats.Counter
> > > > >
> > > > > when using Counter for count metric, use Counter.addCount,intead of
> > old
> > > > > API(Counter.add)
> > > > > when using Counter for latency metric, use Counter.addLatency
> > > > >
> > > > > "Release"
> > > > >
> > > > > because it change the API,so we don't cherry pick this into released
> > > > > branches
> > > > > so it should go into a major release, like Release-4.16, then we can
> > > > update
> > > > > its implementation in other projects, like pulsar
> > > > >
> > > > >
> > > > >
> > > > > What do you think about it?
> > > > >
> > > >
> > >
> >
Re: [DISCUSS] BP-58 Change the API for org.apache.bookkeeper.stats.Counter
Posted by Yong Zhang <zh...@gmail.com>.
LGTM.
Best,
Yong
On Mon, 26 Sept 2022 at 15:56, wenbing shen <ol...@gmail.com>
wrote:
> +1 （non-binding）
>
> Thanks.
> Wenbing Shen
>
> ZhangJian He <sh...@gmail.com> 于2022年9月26日周一 11:15写道：
>
> > +1 （non-binding）
> >
> > Thanks
> > ZhangJian He
> >
> > On Sun, 25 Sept 2022 at 20:45, Enrico Olivelli <eo...@gmail.com>
> > wrote:
> >
> > > I agree
> > >
> > > Enrico
> > >
> > > Il Dom 25 Set 2022, 03:09 steven lu <lu...@gmail.com> ha
> scritto:
> > >
> > > > Hi BookKeepers, I've changed the API for
> > > > org.apache.bookkeeper.stats.Counter:
> > > > BP-58 <https://github.com/apache/bookkeeper/issues/3502>
> > > >
> > > >
> > > > "Motivation"
> > > >
> > > > The latency of the OpStatsLogger.registerSuccessfulEvent calculation
> is
> > > to
> > > > convert the time to milliseconds
> > > > but Counter.add nothing to do
> > > > so when using Counter for latency statistics, the time unit and
> > > > OpStatsLogger are not unified, which is easy to be misleading.
> > > > then we unified latency metric unit
> > > >
> > > > "Proposal"
> > > >
> > > > We already have the Counter metric for counting , we just need to
> > > refactor
> > > > it and expose it as a public method.
> > > > My idea is to change the API of org.apache.bookkeeper.stats.Counter
> > > >
> > > > when using Counter for count metric, use Counter.addCount,intead of
> old
> > > > API(Counter.add)
> > > > when using Counter for latency metric, use Counter.addLatency
> > > >
> > > > "Release"
> > > >
> > > > because it change the API,so we don't cherry pick this into released
> > > > branches
> > > > so it should go into a major release, like Release-4.16, then we can
> > > update
> > > > its implementation in other projects, like pulsar
> > > >
> > > >
> > > >
> > > > What do you think about it?
> > > >
> > >
> >
>
Re: [DISCUSS] BP-58 Change the API for org.apache.bookkeeper.stats.Counter
Posted by wenbing shen <ol...@gmail.com>.
+1 （non-binding）
Thanks.
Wenbing Shen
ZhangJian He <sh...@gmail.com> 于2022年9月26日周一 11:15写道：
> +1 （non-binding）
>
> Thanks
> ZhangJian He
>
> On Sun, 25 Sept 2022 at 20:45, Enrico Olivelli <eo...@gmail.com>
> wrote:
>
> > I agree
> >
> > Enrico
> >
> > Il Dom 25 Set 2022, 03:09 steven lu <lu...@gmail.com> ha scritto:
> >
> > > Hi BookKeepers, I've changed the API for
> > > org.apache.bookkeeper.stats.Counter:
> > > BP-58 <https://github.com/apache/bookkeeper/issues/3502>
> > >
> > >
> > > "Motivation"
> > >
> > > The latency of the OpStatsLogger.registerSuccessfulEvent calculation is
> > to
> > > convert the time to milliseconds
> > > but Counter.add nothing to do
> > > so when using Counter for latency statistics, the time unit and
> > > OpStatsLogger are not unified, which is easy to be misleading.
> > > then we unified latency metric unit
> > >
> > > "Proposal"
> > >
> > > We already have the Counter metric for counting , we just need to
> > refactor
> > > it and expose it as a public method.
> > > My idea is to change the API of org.apache.bookkeeper.stats.Counter
> > >
> > > when using Counter for count metric, use Counter.addCount,intead of old
> > > API(Counter.add)
> > > when using Counter for latency metric, use Counter.addLatency
> > >
> > > "Release"
> > >
> > > because it change the API,so we don't cherry pick this into released
> > > branches
> > > so it should go into a major release, like Release-4.16, then we can
> > update
> > > its implementation in other projects, like pulsar
> > >
> > >
> > >
> > > What do you think about it?
> > >
> >
>
Re: [DISCUSS] BP-58 Change the API for org.apache.bookkeeper.stats.Counter
Posted by ZhangJian He <sh...@gmail.com>.
+1 （non-binding）
Thanks
ZhangJian He
On Sun, 25 Sept 2022 at 20:45, Enrico Olivelli <eo...@gmail.com> wrote:
> I agree
>
> Enrico
>
> Il Dom 25 Set 2022, 03:09 steven lu <lu...@gmail.com> ha scritto:
>
> > Hi BookKeepers, I've changed the API for
> > org.apache.bookkeeper.stats.Counter:
> > BP-58 <https://github.com/apache/bookkeeper/issues/3502>
> >
> >
> > "Motivation"
> >
> > The latency of the OpStatsLogger.registerSuccessfulEvent calculation is
> to
> > convert the time to milliseconds
> > but Counter.add nothing to do
> > so when using Counter for latency statistics, the time unit and
> > OpStatsLogger are not unified, which is easy to be misleading.
> > then we unified latency metric unit
> >
> > "Proposal"
> >
> > We already have the Counter metric for counting , we just need to
> refactor
> > it and expose it as a public method.
> > My idea is to change the API of org.apache.bookkeeper.stats.Counter
> >
> > when using Counter for count metric, use Counter.addCount,intead of old
> > API(Counter.add)
> > when using Counter for latency metric, use Counter.addLatency
> >
> > "Release"
> >
> > because it change the API,so we don't cherry pick this into released
> > branches
> > so it should go into a major release, like Release-4.16, then we can
> update
> > its implementation in other projects, like pulsar
> >
> >
> >
> > What do you think about it?
> >
>
Re: [DISCUSS] BP-58 Change the API for org.apache.bookkeeper.stats.Counter
Posted by Enrico Olivelli <eo...@gmail.com>.
I agree
Enrico
Il Dom 25 Set 2022, 03:09 steven lu <lu...@gmail.com> ha scritto:
> Hi BookKeepers, I've changed the API for
> org.apache.bookkeeper.stats.Counter:
> BP-58 <https://github.com/apache/bookkeeper/issues/3502>
>
>
> "Motivation"
>
> The latency of the OpStatsLogger.registerSuccessfulEvent calculation is to
> convert the time to milliseconds
> but Counter.add nothing to do
> so when using Counter for latency statistics, the time unit and
> OpStatsLogger are not unified, which is easy to be misleading.
> then we unified latency metric unit
>
> "Proposal"
>
> We already have the Counter metric for counting , we just need to refactor
> it and expose it as a public method.
> My idea is to change the API of org.apache.bookkeeper.stats.Counter
>
> when using Counter for count metric, use Counter.addCount,intead of old
> API(Counter.add)
> when using Counter for latency metric, use Counter.addLatency
>
> "Release"
>
> because it change the API,so we don't cherry pick this into released
> branches
> so it should go into a major release, like Release-4.16, then we can update
> its implementation in other projects, like pulsar
>
>
>
> What do you think about it?
>