>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 <z...@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 <xiangy...@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 > > <yubiao.f...@streamnative.io.invalid> wrote: > > > > > > Hi Donglai, Mattison > > > > > > I agree with @Mattison > > > > > > Thanks > > > Yubiao Feng > > > > > > On Mon, Aug 21, 2023 at 8:50 PM <mattisonc...@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 <labulad...@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 > > > >
