Skip to content

Conversation

@thetumbled
Copy link
Member

@thetumbled thetumbled commented Sep 18, 2025

PIP: #24771

Motivation

When the retention of a large topic is reduced, a significant number of ledgers need to be deleted. Since this deletion operation is not rate-limited, it results in ZooKeeper (ZK) latency of several minutes, as shown below:
image

Modifications

Add rate limit feature for deleting ledgers.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

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:

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

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: thetumbled#75

@thetumbled thetumbled changed the title [improve][PIP] Rate limit for deleting ledger to alleviate the zk [improve][PIP] Rate limit for deleting ledger to alleviate the zk pressure. Sep 18, 2025
@apache apache deleted a comment from github-actions bot Sep 18, 2025
@github-actions github-actions bot added doc-required Your PR changes impact docs and you will update later. and removed doc-label-missing labels Sep 18, 2025
@thetumbled thetumbled self-assigned this Sep 22, 2025
@thetumbled thetumbled changed the title [improve][PIP] Rate limit for deleting ledger to alleviate the zk pressure. [improve][broker] PIP-444: Rate limit for deleting ledger to alleviate the zk pressure. Sep 22, 2025
@thetumbled thetumbled requested a review from lhotari September 23, 2025 08:54
Copy link
Contributor

@Denovo1998 Denovo1998 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@codelipenghui
Copy link
Contributor

We do have batch operation to the metadata service. If the deletion option happened concurrently, it should be already batched in one request?

@thetumbled
Copy link
Member Author

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

ti Please enter the commit message for your changes. Lines starting
@BewareMyPower
Copy link
Contributor

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

@thetumbled
Copy link
Member Author

thetumbled commented Sep 29, 2025

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 feature #13043 cannot help resolve this issue.

fengwenzhi added 2 commits September 29, 2025 17:03
@BewareMyPower
Copy link
Contributor

Please update the original document #24771 as well, then you can start the vote

@thetumbled
Copy link
Member Author

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.

@thetumbled thetumbled added this to the 4.2.0 milestone Oct 11, 2025
@thetumbled thetumbled merged commit ee33c99 into apache:master Oct 11, 2025
66 of 69 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-required Your PR changes impact docs and you will update later.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants