add rate limit for zk read rate in gc.

[thetumbled]

Motivation

Each time the bookie gc is triggered, the read latency of zookeeper soars high to tens of seconds, threatening the stability of the cluster.

The unlimited zookeeper read operation in gc is responsible for the issue.

Changes

Add config gcMetadataOpRateLimit to limit the read operation rate in gc.

[thetumbled]

PTAL, thanks. @StevenLuMT @hezhangjian @hangc0276 @nodece @zymap

[StevenLuMT]

I think adding a gcRateLimiter doesn't fundamentally solve the problem, nor is it practically feasible to implement or limit the number of calls.

I think it could be more practical to break down the rate limits into several categories: rate limits for GC calls to ZooKeeper, rate limits for GC file reads, rate limits for GC file writes, or rate limits for other GC resources.

[thetumbled]

I think adding a gcRateLimiter doesn't fundamentally solve the problem, nor is it practically feasible to implement or limit the number of calls.

Why? I believe adding a rate limiter and acquiring it prior to each zk operation would suffice.

I think it could be more practical to break down the rate limits into several categories: rate limits for GC calls to ZooKeeper, rate limits for GC file reads, rate limits for GC file writes, or rate limits for other GC resources.

We haven't encountered any issues with GC file reads or writes. The only thing I need to limit is the rate of zk reads.

[nodece]

Overall, LGTM.

[StevenLuMT]

LGTM: The previous definition was a bit broad. Now you have sunk to MetadataOpRateLimit, which I understand is still useful and configurable.

[StevenLuMT]

By the way, is it possible to add a testcase? If it is possible, please add a testcase to verify the correctness of the function. Thank you @thetumbled

[StevenLuMT]

rerun failure checks

[Copilot]

Pull Request Overview

This PR adds rate limiting for ZooKeeper read operations during garbage collection to prevent ZooKeeper latency spikes that threaten cluster stability. The unlimited ZooKeeper operations during GC were causing read latencies to reach tens of seconds.

• Adds a new configuration parameter gcZkOpRateLimit with a default value of 1000 operations per second
• Implements rate limiting using Google Guava's RateLimiter in the ScanAndCompareGarbageCollector
• Applies rate limiting to ZooKeeper read operations during ledger metadata scanning and overreplicated ledger processing

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
ServerConfiguration.java	Adds configuration parameter and getter/setter methods for GC ZooKeeper operation rate limiting
ScanAndCompareGarbageCollector.java	Implements rate limiting using RateLimiter for ZooKeeper operations during garbage collection

Comments suppressed due to low confidence (1)

bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java:495

• The parameter name gcRateLimit in the javadoc doesn't match the actual parameter name gcRateLimit used in the method signature. Consider using a more descriptive name like gcZkOpRateLimit to match the configuration constant and method name.

     * @param gcRateLimit

[thetumbled]

By the way, is it possible to add a testcase? If it is possible, please add a testcase to verify the correctness of the function. Thank you @thetumbled

A test case has been added to verify the correctness of the rate limit feature, PTAL, thanks.

[thetumbled]

rerun failure checks

[StevenLuMT]

LGTM

[lhotari]

• Please check the configuration guidelines at: https://bookkeeper.apache.org/community/coding-guide/#configuration

• This configuration should also be added to the default conf/bk_server.conf and site3/website/docs/reference/config.md.

• Names should be thought through from the point of view of the person using the config.
• The default values should be thought as best value for people who runs the program without tuning parameters.
• All configuration settings should be added to default configuration file and documented.

• The new configuration should be cross-referenced with the existing "compaction" rate limiting settings since it will also impact the rate.

[eolivelli]

LGTM

[lhotari]

LGTM