You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "thetumbled (via GitHub)" <gi...@apache.org> on 2025/09/18 10:01:13 UTC
[PR] [improve][PIP] Rate limit for deleting ledger to alleviate the zk [pulsar]
thetumbled opened a new pull request, #24760:
URL: https://github.com/apache/pulsar/pull/24760
<!--
### Contribution Checklist
- PR title format should be *[type][component] summary*. For details, see *[Guideline - Pulsar PR Naming Convention](https://pulsar.apache.org/contribute/develop-semantic-title/)*.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
-->
<!-- Either this PR fixes an issue, -->
Fixes #xyz
<!-- or this PR is one task of an issue -->
Main Issue: #xyz
<!-- If the PR belongs to a PIP, please add the PIP link here -->
PIP: #xyz
<!-- Details of when a PIP is required and how the PIP process work, please see: https://github.com/apache/pulsar/blob/master/pip/README.md -->
### Motivation
<!-- Explain here the context, and why you're making that change. What is the problem you're trying to solve. -->
### Modifications
<!-- Describe the modifications you've done. -->
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
*(Please pick either of the following options)*
This change is a trivial rework / code cleanup without any test coverage.
*(or)*
This change is already covered by existing tests, such as *(please describe tests)*.
*(or)*
This change added tests and can be verified as follows:
*(example:)*
- *Added integration tests for end-to-end deployment with large payloads (10MB)*
- *Extended integration test for recovery after broker failure*
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: <!-- ENTER URL HERE -->
<!--
After opening this PR, the build in apache/pulsar will fail and instructions will
be provided for opening a PR in the PR author's forked repository.
apache/pulsar pull requests should be first tested in your own fork since the
apache/pulsar CI based on GitHub Actions has constrained resources and quota.
GitHub Actions provides separate quota for pull requests that are executed in
a forked repository.
The tests will be run in the forked repository until all PR review comments have
been handled, the tests pass and the PR is approved by a reviewer.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#issuecomment-3349946631
Please update the original document #24771 as well, then you can start the vote
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2387386776
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2202,6 +2202,17 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece
doc = "Rate limit the amount of writes per second generated by consumer acking the messages"
)
private double managedLedgerDefaultMarkDeleteRateLimit = 1.0;
+ @FieldContext(
+ category = CATEGORY_STORAGE_ML,
+ doc = "Max number of concurrent requests for deleting ledgers at broker level"
+ )
+ private int managedLedgerDeleteMaxConcurrentRequests = 1000;
+ @FieldContext(
+ category = CATEGORY_STORAGE_ML,
+ doc = "Number of threads to be used for deleting ledgers at broker level, "
+ + "default is the number of CPU cores"
+ )
+ private int managedLedgerDeleteThreadPoolSize = Runtime.getRuntime().availableProcessors();
Review Comment:
It looks like we should use a single thread.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#issuecomment-3344653324
> We do have batch operation to the metadata service. If the deletion option happened concurrently, it should be already batched in one request?
I am afraid that there is no batch feature for `bookKeeper.asyncDeleteLedger`. Do you know anything related to this? @lhotari @BewareMyPower
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2387080113
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2202,6 +2202,16 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece
doc = "Rate limit the amount of writes per second generated by consumer acking the messages"
)
private double managedLedgerDefaultMarkDeleteRateLimit = 1.0;
+ @FieldContext(
+ category = CATEGORY_STORAGE_ML,
+ doc = "Max number of concurrent requests for deleting ledgers at broker level"
+ )
+ private int managedLedgerDeleteMaxConcurrentRequests = 1000;
+ @FieldContext(
+ category = CATEGORY_STORAGE_ML,
+ doc = "Number of threads to be used for deleting ledgers at broker level"
+ )
+ private int managedLedgerDeleteThreadPoolSize = 5;
Review Comment:
You should use the CPU cores.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -821,6 +840,9 @@ public CompletableFuture<Void> closeAsync() {
try {
log.info("Shutting down Pulsar Broker service");
+ // shutdown executor for ledger deletion
+ ledgerDeletionExecutorProvider.shutdownNow();
Review Comment:
Please add NPE check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#issuecomment-3350232363
> Please update the original document #24771 as well, then you can start the vote
Updated, and here is the link to the vote thread: https://lists.apache.org/thread/2dop6kyvrpt6ztz9pvylyl8o2syvh9kw.
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#issuecomment-3344792637
I think @codelipenghui mentioned this feature: https://github.com/apache/pulsar/pull/13043, while `asyncDeleteLedger` involves a metadata store operation.
But from my perspective, the batching feature seems to be enabled by default, so I don't think it can resolve the issue mentioned in PIP-444
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2371359584
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -468,6 +472,19 @@ public BrokerService(PulsarService pulsar, EventLoopGroup eventLoopGroup) throws
.getBrokerEntryPayloadProcessors(), BrokerService.class.getClassLoader());
this.bundlesQuotas = new BundlesQuotas(pulsar);
+ if (pulsar.getConfiguration().getManagedLedgerDeleteRateLimit() > 0) {
+ log.info("Setting managed ledger deletion rate limit to {}, concurrency {}",
+ pulsar.getConfiguration().getManagedLedgerDeleteRateLimit(),
+ pulsar.getConfiguration().getManagedLedgerDeleteConcurrency());
+ this.ledgerDeletionRateLimiter = RateLimiter.create(
+ pulsar.getConfiguration().getManagedLedgerDeleteRateLimit());
+ this.ledgerDeletionExecutorProvider = new ExecutorProvider(
+ pulsar.getConfiguration().getManagedLedgerDeleteConcurrency(),
Review Comment:
You are right, I have used the semaphore to limit the concurrency.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2377587807
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -2058,6 +2077,8 @@ public CompletableFuture<ManagedLedgerConfig> getManagedLedgerConfig(@NonNull To
managedLedgerConfig.setThrottleMarkDelete(persistencePolicies.getManagedLedgerMaxMarkDeleteRate() >= 0
? persistencePolicies.getManagedLedgerMaxMarkDeleteRate()
: serviceConfig.getManagedLedgerDefaultMarkDeleteRateLimit());
+ managedLedgerConfig.setLedgerDeletionSemaphore(this.ledgerDeletionSemaphore);
+ managedLedgerConfig.setLedgerDeleteExecutor(this.ledgerDeletionExecutorProvider.getExecutor());
Review Comment:
fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][PIP] Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2360959383
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -569,6 +576,7 @@ protected synchronized void initializeBookKeeper(final ManagedLedgerInitializeLe
public void operationComplete(Void v, Stat stat) {
ledgersStat = stat;
emptyLedgersToBeDeleted.forEach(ledgerId -> {
+ deleteLedgerRateLimiter.acquire();
Review Comment:
This is a blocking operation. We cannot block the thread in this callback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][PIP] Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2361299952
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -569,6 +576,7 @@ protected synchronized void initializeBookKeeper(final ManagedLedgerInitializeLe
public void operationComplete(Void v, Stat stat) {
ledgersStat = stat;
emptyLedgersToBeDeleted.forEach(ledgerId -> {
+ deleteLedgerRateLimiter.acquire();
Review Comment:
@thetumbled Since you have placed the rate limiter's acquire before each call to asyncDeleteLedger, I'd recommend extracting a method that removes the code duplication (includes the logic for rate limiting and deleting) and makes it easier to address the problem of blocking code, since that needs more changes.
There would need to be a separate executor for triggering the asyncDeleteLedger jobs so that the BK callback thread isn't blocked.
Rate limiting is not necessarily alone the most useful way of handling the problem.
It might be necessary to also limit concurrency. A semaphore would be needed for that purpose. There should be both a rate limit and semaphore for fully addressing the problem. One might want to disable rate limiting, but allow 100 concurrent deletions to happen at a time; or have a combination.
Instead of having **ledger** level rate limiting and concurrency limits, I'd recommend placing this in the ManagedLedgerFactoryImpl level so that it would be applied across the whole broker. I don't see how the limits could be reliable when they are at the ledger level.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][PIP] Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2361299952
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -569,6 +576,7 @@ protected synchronized void initializeBookKeeper(final ManagedLedgerInitializeLe
public void operationComplete(Void v, Stat stat) {
ledgersStat = stat;
emptyLedgersToBeDeleted.forEach(ledgerId -> {
+ deleteLedgerRateLimiter.acquire();
Review Comment:
@thetumbled Since you have placed the rate limiter's acquire before each call to asyncDeleteLedger, I'd recommend extracting a method that removes the code duplication and makes it easier to address the problem of blocking code.
There would need to be a separate executor for triggering the asyncDeleteLedger jobs so that the BK callback thread isn't blocked.
Rate limiting is not necessarily alone the most useful way of handling the problem.
It might be necessary to also limit concurrency. A semaphore would be needed for that purpose. There should be both a rate limit and semaphore for fully addressing the problem. One might want to disable rate limiting, but allow 100 concurrent deletions to happen at a time; or have a combination.
Instead of having **ledger** level rate limiting and concurrency limits, I'd recommend placing this in the ManagedLedgerFactoryImpl level so that it would be applied across the whole broker. I don't see how the limits could be reliable when they are at the ledger level.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "Denovo1998 (via GitHub)" <gi...@apache.org>.
Denovo1998 commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2375862877
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -402,6 +408,10 @@ public ManagedLedgerImpl(ManagedLedgerFactoryImpl factory, BookKeeper bookKeeper
this.minBacklogEntriesForCaching = config.getMinimumBacklogEntriesForCaching();
this.maxBacklogBetweenCursorsForCaching = config.getMaxBacklogBetweenCursorsForCaching();
this.managedLedgerAttributes = new ManagedLedgerAttributes(this);
+ if (config.getLedgerDeletionSemaphore() != null) {
+ this.deleteLedgerSemaphore = config.getLedgerDeletionSemaphore();
+ this.deleteLedgerExecutor = config.getLedgerDeleteExecutor();
Review Comment:
The initialization logic should validate that both the semaphore and executor are provided together?
Defensive programming:
```java
if (config.getLedgerDeletionSemaphore() != null) {
this.deleteLedgerSemaphore = config.getLedgerDeletionSemaphore();
this.deleteLedgerExecutor = config.getLedgerDeleteExecutor();
if (this.deleteLedgerExecutor == null) {
throw new IllegalArgumentException("LedgerDeleteExecutor must be provided when LedgerDeletionSemaphore is configured");
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "Denovo1998 (via GitHub)" <gi...@apache.org>.
Denovo1998 commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2375849887
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -2058,6 +2077,8 @@ public CompletableFuture<ManagedLedgerConfig> getManagedLedgerConfig(@NonNull To
managedLedgerConfig.setThrottleMarkDelete(persistencePolicies.getManagedLedgerMaxMarkDeleteRate() >= 0
? persistencePolicies.getManagedLedgerMaxMarkDeleteRate()
: serviceConfig.getManagedLedgerDefaultMarkDeleteRateLimit());
+ managedLedgerConfig.setLedgerDeletionSemaphore(this.ledgerDeletionSemaphore);
+ managedLedgerConfig.setLedgerDeleteExecutor(this.ledgerDeletionExecutorProvider.getExecutor());
Review Comment:
When `getManagedLedgerDeleteMaxConcurrentRequests()` is 0 or negative, `ledgerDeletionExecutorProvider` is set to null in the constructor. However, there is no null check here when calling `this.ledgerDeletionExecutorProvider.getExecutor()`, which will cause a `NullPointerException`?
```java
managedLedgerConfig.setLedgerDeleteExecutor(this.ledgerDeletionExecutorProvider != null ?this.ledgerDeletionExecutorProvider.getExecutor() : null);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#issuecomment-3343916599
We do have batch operation to the metadata service. If the deletion option happened concurrently, it should be already batched in one request?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2387348162
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2202,6 +2202,17 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece
doc = "Rate limit the amount of writes per second generated by consumer acking the messages"
)
private double managedLedgerDefaultMarkDeleteRateLimit = 1.0;
+ @FieldContext(
+ category = CATEGORY_STORAGE_ML,
+ doc = "Max number of concurrent requests for deleting ledgers at broker level"
+ )
+ private int managedLedgerDeleteMaxConcurrentRequests = 1000;
+ @FieldContext(
+ category = CATEGORY_STORAGE_ML,
+ doc = "Number of threads to be used for deleting ledgers at broker level, "
+ + "default is the number of CPU cores"
+ )
+ private int managedLedgerDeleteThreadPoolSize = Runtime.getRuntime().availableProcessors();
Review Comment:
@nodece I noticed your comment [here](https://github.com/apache/pulsar/pull/24760#discussion_r2387080113). However, using CPU cores as the default thread number is not always a silver bullet.
The only task executed by this executor is:
```java
deleteLedgerSemaphore.acquire();
bookKeeper.asyncDeleteLedger(ledgerId, cbWrapper, ctx);
```
`asyncDeleteLedger` is an asynchronous operation. Running asynchronous tasks in parallel is not a good practice. We only need a separated thread to avoid blocking the metadata store I/O thread by `acquire`. In this case, even if there are N threads, all of them will be blocked at `acquire`, which does not get the benefits from multi-threading as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2387219976
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -821,6 +840,9 @@ public CompletableFuture<Void> closeAsync() {
try {
log.info("Shutting down Pulsar Broker service");
+ // shutdown executor for ledger deletion
+ ledgerDeletionExecutorProvider.shutdownNow();
Review Comment:
fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2369017336
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedgerConfig.java:
##########
@@ -61,6 +64,8 @@ public class ManagedLedgerConfig {
private int metadataMaxEntriesPerLedger = 50000;
private int ledgerRolloverTimeout = 4 * 3600;
private double throttleMarkDelete = 0;
+ private RateLimiter ledgerDeletaRateLimiter;
Review Comment:
typo here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled merged PR #24760:
URL: https://github.com/apache/pulsar/pull/24760
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "Denovo1998 (via GitHub)" <gi...@apache.org>.
Denovo1998 commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2375873391
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -3408,6 +3418,32 @@ private void asyncDeleteLedgerWithRetry(CompletableFuture<Void> future, long led
}, null);
}
+ /**
+ * Delete a ledger asynchronously, applying a concurrency limit if configured.
+ * @param ledgerId
+ * @param cb
+ * @param ctx
+ */
+ private void asyncDeleteLedgerWithConcurrencyLimit(long ledgerId, org.apache.bookkeeper.client.AsyncCallback.DeleteCallback cb,
+ Object ctx) {
+ if (deleteLedgerSemaphore != null) {
+ AsyncCallback.DeleteCallback cbWrapper = (rc, ctx1) -> {
+ deleteLedgerSemaphore.release();
+ cb.deleteComplete(rc, ctx1);
+ };
+ deleteLedgerExecutor.execute(() -> {
+ try {
+ deleteLedgerSemaphore.acquire();
+ } catch (InterruptedException e) {
+ log.error("[{}] Interrupted while waiting to delete ledger {}", name, ledgerId, e);
+ }
+ bookKeeper.asyncDeleteLedger(ledgerId, cbWrapper, ctx);
Review Comment:
When semaphore acquisition is interrupted, the code logs the error but continues with the deletion. This could lead to exceeding the concurrency limit.
Looks a bit strange?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -2058,6 +2077,8 @@ public CompletableFuture<ManagedLedgerConfig> getManagedLedgerConfig(@NonNull To
managedLedgerConfig.setThrottleMarkDelete(persistencePolicies.getManagedLedgerMaxMarkDeleteRate() >= 0
? persistencePolicies.getManagedLedgerMaxMarkDeleteRate()
: serviceConfig.getManagedLedgerDefaultMarkDeleteRateLimit());
+ managedLedgerConfig.setLedgerDeletionSemaphore(this.ledgerDeletionSemaphore);
+ managedLedgerConfig.setLedgerDeleteExecutor(this.ledgerDeletionExecutorProvider.getExecutor());
Review Comment:
When `getManagedLedgerDeleteMaxConcurrentRequests()` is 0 or negative, `ledgerDeletionExecutorProvider` is set to null in the constructor. However, there is no null check here when calling `this.ledgerDeletionExecutorProvider.getExecutor()`, which will cause a `NullPointerException`.
```java
managedLedgerConfig.setLedgerDeleteExecutor(this.ledgerDeletionExecutorProvider != null ?this.ledgerDeletionExecutorProvider.getExecutor() : null);
```
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -402,6 +408,10 @@ public ManagedLedgerImpl(ManagedLedgerFactoryImpl factory, BookKeeper bookKeeper
this.minBacklogEntriesForCaching = config.getMinimumBacklogEntriesForCaching();
this.maxBacklogBetweenCursorsForCaching = config.getMaxBacklogBetweenCursorsForCaching();
this.managedLedgerAttributes = new ManagedLedgerAttributes(this);
+ if (config.getLedgerDeletionSemaphore() != null) {
+ this.deleteLedgerSemaphore = config.getLedgerDeletionSemaphore();
+ this.deleteLedgerExecutor = config.getLedgerDeleteExecutor();
Review Comment:
The initialization logic should validate that both the semaphore and executor are provided together.
Defensive programming:
```java
if (config.getLedgerDeletionSemaphore() != null) {
this.deleteLedgerSemaphore = config.getLedgerDeletionSemaphore();
this.deleteLedgerExecutor = config.getLedgerDeleteExecutor();
if (this.deleteLedgerExecutor == null) {
throw new IllegalArgumentException("LedgerDeleteExecutor must be provided when LedgerDeletionSemaphore is configured");
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2377590865
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -3408,6 +3418,32 @@ private void asyncDeleteLedgerWithRetry(CompletableFuture<Void> future, long led
}, null);
}
+ /**
+ * Delete a ledger asynchronously, applying a concurrency limit if configured.
+ * @param ledgerId
+ * @param cb
+ * @param ctx
+ */
+ private void asyncDeleteLedgerWithConcurrencyLimit(long ledgerId, org.apache.bookkeeper.client.AsyncCallback.DeleteCallback cb,
+ Object ctx) {
+ if (deleteLedgerSemaphore != null) {
+ AsyncCallback.DeleteCallback cbWrapper = (rc, ctx1) -> {
+ deleteLedgerSemaphore.release();
+ cb.deleteComplete(rc, ctx1);
+ };
+ deleteLedgerExecutor.execute(() -> {
+ try {
+ deleteLedgerSemaphore.acquire();
+ } catch (InterruptedException e) {
+ log.error("[{}] Interrupted while waiting to delete ledger {}", name, ledgerId, e);
+ }
+ bookKeeper.asyncDeleteLedger(ledgerId, cbWrapper, ctx);
Review Comment:
This is a point that needs to be discussed: Should we proceed to send the delete request? @lhotari
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2369007825
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -468,6 +472,19 @@ public BrokerService(PulsarService pulsar, EventLoopGroup eventLoopGroup) throws
.getBrokerEntryPayloadProcessors(), BrokerService.class.getClassLoader());
this.bundlesQuotas = new BundlesQuotas(pulsar);
+ if (pulsar.getConfiguration().getManagedLedgerDeleteRateLimit() > 0) {
+ log.info("Setting managed ledger deletion rate limit to {}, concurrency {}",
+ pulsar.getConfiguration().getManagedLedgerDeleteRateLimit(),
+ pulsar.getConfiguration().getManagedLedgerDeleteConcurrency());
+ this.ledgerDeletionRateLimiter = RateLimiter.create(
+ pulsar.getConfiguration().getManagedLedgerDeleteRateLimit());
+ this.ledgerDeletionExecutorProvider = new ExecutorProvider(
+ pulsar.getConfiguration().getManagedLedgerDeleteConcurrency(),
Review Comment:
This solution doesn't work for the concurrency. `bookKeeper.asyncDeleteLedger` is an asynchronous call and the purpose should be to limit how many deletions are "in flight".
A semaphore could handle this so that you acquire the permit before making the call and release it after the deletion has completed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2384868134
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -468,6 +474,19 @@ public BrokerService(PulsarService pulsar, EventLoopGroup eventLoopGroup) throws
.getBrokerEntryPayloadProcessors(), BrokerService.class.getClassLoader());
this.bundlesQuotas = new BundlesQuotas(pulsar);
+ if (pulsar.getConfiguration().getManagedLedgerDeleteMaxConcurrentRequests() > 0) {
+ log.info("Setting managed ledger deletion max concurrent requests to {} and executor pool size to {}",
+ pulsar.getConfiguration().getManagedLedgerDeleteMaxConcurrentRequests(),
+ pulsar.getConfiguration().getManagedLedgerDeleteThreadPoolSize());
+ this.ledgerDeletionSemaphore = new Semaphore(
+ pulsar.getConfiguration().getManagedLedgerDeleteMaxConcurrentRequests());
+ this.ledgerDeletionExecutorProvider = new ExecutorProvider(
+ pulsar.getConfiguration().getManagedLedgerDeleteThreadPoolSize(),
+ "pulsar-ledger-deletion");
Review Comment:
We should shutdown the `ExecutorProvider`
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedgerConfig.java:
##########
@@ -23,7 +23,11 @@
import java.time.Clock;
import java.util.Arrays;
import java.util.Map;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Semaphore;
import java.util.concurrent.TimeUnit;
+
+import com.google.common.util.concurrent.RateLimiter;
Review Comment:
seems it violates the checkstyle check?
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -3408,6 +3418,32 @@ private void asyncDeleteLedgerWithRetry(CompletableFuture<Void> future, long led
}, null);
}
+ /**
+ * Delete a ledger asynchronously, applying a concurrency limit if configured.
+ * @param ledgerId
+ * @param cb
+ * @param ctx
+ */
+ private void asyncDeleteLedgerWithConcurrencyLimit(long ledgerId, org.apache.bookkeeper.client.AsyncCallback.DeleteCallback cb,
+ Object ctx) {
+ if (deleteLedgerSemaphore != null) {
+ AsyncCallback.DeleteCallback cbWrapper = (rc, ctx1) -> {
+ deleteLedgerSemaphore.release();
+ cb.deleteComplete(rc, ctx1);
+ };
+ deleteLedgerExecutor.execute(() -> {
+ try {
+ deleteLedgerSemaphore.acquire();
+ } catch (InterruptedException e) {
+ log.error("[{}] Interrupted while waiting to delete ledger {}", name, ledgerId, e);
+ }
+ bookKeeper.asyncDeleteLedger(ledgerId, cbWrapper, ctx);
Review Comment:
I suggest logging an error and exit, and there is no need to print the whole stack.
An `InterruptedException` is thrown when:
- the future returned by `Executor#submit` is called with `.cancel(true)` or the semaphore is called.
- or when the `acquire` returns, the thread has already exited (e.g. `executor.shutdown()` is called)
P.S. you can check the following code snippet to see the behavior:
```java
final var executor = Executors.newSingleThreadExecutor();
final var semaphore = new Semaphore(1);
semaphore.acquire();
final var future = executor.submit(() -> {
try {
semaphore.acquire();
System.out.println("done");
} catch (InterruptedException e) {
System.out.println("interrupted: " + Thread.currentThread().isInterrupted());
}
});
Thread.sleep(300);
future.cancel(true);
executor.shutdown();
semaphore.release();
Thread.sleep(300);
semaphore.release();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2387468201
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2202,6 +2202,17 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece
doc = "Rate limit the amount of writes per second generated by consumer acking the messages"
)
private double managedLedgerDefaultMarkDeleteRateLimit = 1.0;
+ @FieldContext(
+ category = CATEGORY_STORAGE_ML,
+ doc = "Max number of concurrent requests for deleting ledgers at broker level"
+ )
+ private int managedLedgerDeleteMaxConcurrentRequests = 1000;
+ @FieldContext(
+ category = CATEGORY_STORAGE_ML,
+ doc = "Number of threads to be used for deleting ledgers at broker level, "
+ + "default is the number of CPU cores"
+ )
+ private int managedLedgerDeleteThreadPoolSize = Runtime.getRuntime().availableProcessors();
Review Comment:
Yes, single thread would be better, and I don't think we need to make it configurable. (question: in which cases will you increase the number of threads)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2386551950
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -468,6 +474,19 @@ public BrokerService(PulsarService pulsar, EventLoopGroup eventLoopGroup) throws
.getBrokerEntryPayloadProcessors(), BrokerService.class.getClassLoader());
this.bundlesQuotas = new BundlesQuotas(pulsar);
+ if (pulsar.getConfiguration().getManagedLedgerDeleteMaxConcurrentRequests() > 0) {
+ log.info("Setting managed ledger deletion max concurrent requests to {} and executor pool size to {}",
+ pulsar.getConfiguration().getManagedLedgerDeleteMaxConcurrentRequests(),
+ pulsar.getConfiguration().getManagedLedgerDeleteThreadPoolSize());
+ this.ledgerDeletionSemaphore = new Semaphore(
+ pulsar.getConfiguration().getManagedLedgerDeleteMaxConcurrentRequests());
+ this.ledgerDeletionExecutorProvider = new ExecutorProvider(
+ pulsar.getConfiguration().getManagedLedgerDeleteThreadPoolSize(),
+ "pulsar-ledger-deletion");
Review Comment:
fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2387364439
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2202,6 +2202,17 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece
doc = "Rate limit the amount of writes per second generated by consumer acking the messages"
)
private double managedLedgerDefaultMarkDeleteRateLimit = 1.0;
+ @FieldContext(
+ category = CATEGORY_STORAGE_ML,
+ doc = "Max number of concurrent requests for deleting ledgers at broker level"
+ )
+ private int managedLedgerDeleteMaxConcurrentRequests = 1000;
+ @FieldContext(
+ category = CATEGORY_STORAGE_ML,
+ doc = "Number of threads to be used for deleting ledgers at broker level, "
+ + "default is the number of CPU cores"
+ )
+ private int managedLedgerDeleteThreadPoolSize = Runtime.getRuntime().availableProcessors();
Review Comment:
Yes, we need an appropriate number of threads. How about setting it back to 5?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#issuecomment-3345321350
> I think @codelipenghui mentioned this feature: #13043, while `asyncDeleteLedger` involves a metadata store operation.
>
> But from my perspective, the batching feature seems to be enabled by default, so I don't think it can resolve the issue mentioned in PIP-444
Only when the zk request is issued by broker can this feature takes effect and batch them to alleviate the zk pressure.
However, in this case, the zk requests are sent by the bookie client. Therefore, I'm afraid this feature cannot help resolve this issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2387498538
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2202,6 +2202,17 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece
doc = "Rate limit the amount of writes per second generated by consumer acking the messages"
)
private double managedLedgerDefaultMarkDeleteRateLimit = 1.0;
+ @FieldContext(
+ category = CATEGORY_STORAGE_ML,
+ doc = "Max number of concurrent requests for deleting ledgers at broker level"
+ )
+ private int managedLedgerDeleteMaxConcurrentRequests = 1000;
+ @FieldContext(
+ category = CATEGORY_STORAGE_ML,
+ doc = "Number of threads to be used for deleting ledgers at broker level, "
+ + "default is the number of CPU cores"
+ )
+ private int managedLedgerDeleteThreadPoolSize = Runtime.getRuntime().availableProcessors();
Review Comment:
Built-in is better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2389689685
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2202,6 +2202,17 @@ The max allowed delay for delayed delivery (in milliseconds). If the broker rece
doc = "Rate limit the amount of writes per second generated by consumer acking the messages"
)
private double managedLedgerDefaultMarkDeleteRateLimit = 1.0;
+ @FieldContext(
+ category = CATEGORY_STORAGE_ML,
+ doc = "Max number of concurrent requests for deleting ledgers at broker level"
+ )
+ private int managedLedgerDeleteMaxConcurrentRequests = 1000;
+ @FieldContext(
+ category = CATEGORY_STORAGE_ML,
+ doc = "Number of threads to be used for deleting ledgers at broker level, "
+ + "default is the number of CPU cores"
+ )
+ private int managedLedgerDeleteThreadPoolSize = Runtime.getRuntime().availableProcessors();
Review Comment:
updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][PIP] Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#issuecomment-3306598471
@thetumbled Please add the following content to your PR description and select a checkbox:
```
- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][PIP] Rate limit for deleting ledger to alleviate the zk pressure. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #24760:
URL: https://github.com/apache/pulsar/pull/24760#discussion_r2366874354
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -569,6 +576,7 @@ protected synchronized void initializeBookKeeper(final ManagedLedgerInitializeLe
public void operationComplete(Void v, Stat stat) {
ledgersStat = stat;
emptyLedgersToBeDeleted.forEach(ledgerId -> {
+ deleteLedgerRateLimiter.acquire();
Review Comment:
Good point, i will refactor the implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org