You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pulsar.apache.org by Yubiao Feng <yu...@streamnative.io.INVALID> on 2023/09/25 04:14:26 UTC
Re: [DISCUSS] Unload Rate Limiting during Graceful Shutdown of Pulsar
Hi Donglai, Mattison
I agree with @Mattison
Thanks
Yubiao Feng
On Mon, Aug 21, 2023 at 8:50 PM <ma...@gmail.com> wrote:
>
> Hi,
>
> I agree with this change to improve the stability of the pulsar cluster.
>
> Just one concern. it's better to move this discussion to a new PIP.
> because you wanna introduce a new broker configuration.
> `brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute`
>
> FYI: https://github.com/apache/pulsar/blob/master/pip/README.md
>
> Looking forward this change and thanks for your contribution. :)
>
> Best,
> Mattison
>
>
>
> On 7 Jul 2023 at 15:30 +0800, labuladong <la...@foxmail.com>, wrote:
> > Thanks you guys.
> >
> >
> > I agree that per-minute is better than per-second, which is more
> flexible.
> >
> >
> > I open an issue here:
> >
> >
> > https://github.com/apache/pulsar/issues/20753
> >
> >
> > Regards,
> > donglai
>
Re: [DISCUSS] Unload Rate Limiting during Graceful Shutdown of Pulsar
Posted by Xiangying Meng <xi...@apache.org>.
>I think the RateLimiter can handle it:
https://github.com/apache/pulsar/blob/a1405ea006f175b1bd0b9d28b9444d592fb4a010/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L965-L968
See here. Do you mean we make `maxConcurrentUnloadPerSec` and
`brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute` work
together? What is the meaning of this?
https://github.com/apache/pulsar/blob/a1405ea006f175b1bd0b9d28b9444d592fb4a010/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java#L564-L568
>`maxConcurrentUnloadPerSec ` is for the admin and CLI usage. This
proposal is to add
`brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute ` to the
broker configuration.
Yes, we are adding a broker configuration. That's not an issue, but
adding `brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute` with a
minute-based interval and expecting it to work in conjunction with
`maxConcurrentUnloadPerSec`, which is only used in the CLI, doesn't
make sense.
I understand that what we want to achieve is to have a broker
configuration that limits the concurrency of unloads even when
`maxConcurrentUnloadPerSec` is not set. Instead of adding
`brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute` when
`maxConcurrentUnloadPerSec` is already controlling the concurrency
Thanks,
Xiangying
On Mon, Sep 25, 2023 at 6:45 PM Zike Yang <zi...@apache.org> wrote:
>
> > If we want the
> maximum concurrency per second to be 1, and set the maximum
> concurrency per minute to 60, then the actual maximum concurrency per
> second could be up to 60, which is 60 times larger than our expected
> maximum concurrency.
>
> I think the RateLimiter can handle it:
> https://github.com/apache/pulsar/blob/a1405ea006f175b1bd0b9d28b9444d592fb4a010/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L965-L968
>
> > Secondly, we already have the `maxConcurrentUnloadPerSec`
> configuration, which is provided to the user in the CLI. Adding a
> similar `brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute`
> configuration might confuse users.
>
> `maxConcurrentUnloadPerSec ` is for the admin and CLI usage. This
> proposal is to add
> `brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute ` to the
> broker configuration.
>
>
> Overall, I'm +1 for this proposal. And I agree that we need a new PIP
> for this change.
>
> BR,
> Zike Yang
>
> On Mon, Sep 25, 2023 at 3:54 PM Xiangying Meng <xi...@apache.org> wrote:
> >
> > Hi Donglai, Heesung
> >
> > >brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute=60 is the same as
> > brokerShutdownMaxNumberOfGracefulBundleUnloadPerSec=1 So, the "per-min"
> > config can be more granular.
> >
> > I have some doubts about introducing the
> > `brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute`
> > configuration.
> >
> > Firstly, I also think that a minute is too long. If we want the
> > maximum concurrency per second to be 1, and set the maximum
> > concurrency per minute to 60, then the actual maximum concurrency per
> > second could be up to 60, which is 60 times larger than our expected
> > maximum concurrency. Moreover, if the unload requests are concentrated
> > in the last 10 seconds of the previous minute and the first 10 seconds
> > of the next minute, then the concurrency during this period will
> > exceed our configuration. Such fluctuations are inevitable, but the
> > larger the time span we set, the greater the distortion of the
> > configuration.
> >
> > Secondly, we already have the `maxConcurrentUnloadPerSec`
> > configuration, which is provided to the user in the CLI. Adding a
> > similar `brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute`
> > configuration might confuse users. When designing configuration
> > parameters, we should try to keep it simple and consistent, and avoid
> > introducing unnecessary complexity.
> >
> > Thanks,
> > Xiangying
> >
> > On Mon, Sep 25, 2023 at 12:14 PM Yubiao Feng
> > <yu...@streamnative.io.invalid> wrote:
> > >
> > > Hi Donglai, Mattison
> > >
> > > I agree with @Mattison
> > >
> > > Thanks
> > > Yubiao Feng
> > >
> > > On Mon, Aug 21, 2023 at 8:50 PM <ma...@gmail.com> wrote:
> > >
> > > >
> > > > Hi,
> > > >
> > > > I agree with this change to improve the stability of the pulsar cluster.
> > > >
> > > > Just one concern. it's better to move this discussion to a new PIP.
> > > > because you wanna introduce a new broker configuration.
> > > > `brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute`
> > > >
> > > > FYI: https://github.com/apache/pulsar/blob/master/pip/README.md
> > > >
> > > > Looking forward this change and thanks for your contribution. :)
> > > >
> > > > Best,
> > > > Mattison
> > > >
> > > >
> > > >
> > > > On 7 Jul 2023 at 15:30 +0800, labuladong <la...@foxmail.com>, wrote:
> > > > > Thanks you guys.
> > > > >
> > > > >
> > > > > I agree that per-minute is better than per-second, which is more
> > > > flexible.
> > > > >
> > > > >
> > > > > I open an issue here:
> > > > >
> > > > >
> > > > > https://github.com/apache/pulsar/issues/20753
> > > > >
> > > > >
> > > > > Regards,
> > > > > donglai
> > > >
Re: [DISCUSS] Unload Rate Limiting during Graceful Shutdown of Pulsar
Posted by Zike Yang <zi...@apache.org>.
> If we want the
maximum concurrency per second to be 1, and set the maximum
concurrency per minute to 60, then the actual maximum concurrency per
second could be up to 60, which is 60 times larger than our expected
maximum concurrency.
I think the RateLimiter can handle it:
https://github.com/apache/pulsar/blob/a1405ea006f175b1bd0b9d28b9444d592fb4a010/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L965-L968
> Secondly, we already have the `maxConcurrentUnloadPerSec`
configuration, which is provided to the user in the CLI. Adding a
similar `brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute`
configuration might confuse users.
`maxConcurrentUnloadPerSec ` is for the admin and CLI usage. This
proposal is to add
`brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute ` to the
broker configuration.
Overall, I'm +1 for this proposal. And I agree that we need a new PIP
for this change.
BR,
Zike Yang
On Mon, Sep 25, 2023 at 3:54 PM Xiangying Meng <xi...@apache.org> wrote:
>
> Hi Donglai, Heesung
>
> >brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute=60 is the same as
> brokerShutdownMaxNumberOfGracefulBundleUnloadPerSec=1 So, the "per-min"
> config can be more granular.
>
> I have some doubts about introducing the
> `brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute`
> configuration.
>
> Firstly, I also think that a minute is too long. If we want the
> maximum concurrency per second to be 1, and set the maximum
> concurrency per minute to 60, then the actual maximum concurrency per
> second could be up to 60, which is 60 times larger than our expected
> maximum concurrency. Moreover, if the unload requests are concentrated
> in the last 10 seconds of the previous minute and the first 10 seconds
> of the next minute, then the concurrency during this period will
> exceed our configuration. Such fluctuations are inevitable, but the
> larger the time span we set, the greater the distortion of the
> configuration.
>
> Secondly, we already have the `maxConcurrentUnloadPerSec`
> configuration, which is provided to the user in the CLI. Adding a
> similar `brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute`
> configuration might confuse users. When designing configuration
> parameters, we should try to keep it simple and consistent, and avoid
> introducing unnecessary complexity.
>
> Thanks,
> Xiangying
>
> On Mon, Sep 25, 2023 at 12:14 PM Yubiao Feng
> <yu...@streamnative.io.invalid> wrote:
> >
> > Hi Donglai, Mattison
> >
> > I agree with @Mattison
> >
> > Thanks
> > Yubiao Feng
> >
> > On Mon, Aug 21, 2023 at 8:50 PM <ma...@gmail.com> wrote:
> >
> > >
> > > Hi,
> > >
> > > I agree with this change to improve the stability of the pulsar cluster.
> > >
> > > Just one concern. it's better to move this discussion to a new PIP.
> > > because you wanna introduce a new broker configuration.
> > > `brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute`
> > >
> > > FYI: https://github.com/apache/pulsar/blob/master/pip/README.md
> > >
> > > Looking forward this change and thanks for your contribution. :)
> > >
> > > Best,
> > > Mattison
> > >
> > >
> > >
> > > On 7 Jul 2023 at 15:30 +0800, labuladong <la...@foxmail.com>, wrote:
> > > > Thanks you guys.
> > > >
> > > >
> > > > I agree that per-minute is better than per-second, which is more
> > > flexible.
> > > >
> > > >
> > > > I open an issue here:
> > > >
> > > >
> > > > https://github.com/apache/pulsar/issues/20753
> > > >
> > > >
> > > > Regards,
> > > > donglai
> > >
Re: [DISCUSS] Unload Rate Limiting during Graceful Shutdown of Pulsar
Posted by Xiangying Meng <xi...@apache.org>.
Hi Donglai, Heesung
>brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute=60 is the same as
brokerShutdownMaxNumberOfGracefulBundleUnloadPerSec=1 So, the "per-min"
config can be more granular.
I have some doubts about introducing the
`brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute`
configuration.
Firstly, I also think that a minute is too long. If we want the
maximum concurrency per second to be 1, and set the maximum
concurrency per minute to 60, then the actual maximum concurrency per
second could be up to 60, which is 60 times larger than our expected
maximum concurrency. Moreover, if the unload requests are concentrated
in the last 10 seconds of the previous minute and the first 10 seconds
of the next minute, then the concurrency during this period will
exceed our configuration. Such fluctuations are inevitable, but the
larger the time span we set, the greater the distortion of the
configuration.
Secondly, we already have the `maxConcurrentUnloadPerSec`
configuration, which is provided to the user in the CLI. Adding a
similar `brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute`
configuration might confuse users. When designing configuration
parameters, we should try to keep it simple and consistent, and avoid
introducing unnecessary complexity.
Thanks,
Xiangying
On Mon, Sep 25, 2023 at 12:14 PM Yubiao Feng
<yu...@streamnative.io.invalid> wrote:
>
> Hi Donglai, Mattison
>
> I agree with @Mattison
>
> Thanks
> Yubiao Feng
>
> On Mon, Aug 21, 2023 at 8:50 PM <ma...@gmail.com> wrote:
>
> >
> > Hi,
> >
> > I agree with this change to improve the stability of the pulsar cluster.
> >
> > Just one concern. it's better to move this discussion to a new PIP.
> > because you wanna introduce a new broker configuration.
> > `brokerShutdownMaxNumberOfGracefulBundleUnloadPerMinute`
> >
> > FYI: https://github.com/apache/pulsar/blob/master/pip/README.md
> >
> > Looking forward this change and thanks for your contribution. :)
> >
> > Best,
> > Mattison
> >
> >
> >
> > On 7 Jul 2023 at 15:30 +0800, labuladong <la...@foxmail.com>, wrote:
> > > Thanks you guys.
> > >
> > >
> > > I agree that per-minute is better than per-second, which is more
> > flexible.
> > >
> > >
> > > I open an issue here:
> > >
> > >
> > > https://github.com/apache/pulsar/issues/20753
> > >
> > >
> > > Regards,
> > > donglai
> >