github-actions[bot] commented on code in PR #66977:
URL: https://github.com/apache/doris/pull/66977#discussion_r3820229232
##########
be/src/cloud/cloud_ms_rpc_rate_limiters.cpp:
##########
@@ -139,10 +148,21 @@ int64_t HostLevelMSRpcRateLimiters::limit(MetaServiceRPC
rpc) {
}
auto limiter = _limiters[idx].load();
- if (limiter && limiter->limiter) {
- return limiter->limiter->add(1);
+ if (!limiter) {
+ return 0;
+ }
+ DCHECK(limiter->limiter);
+
+ auto result = dry_run ? limiter->limiter->reserve_with_config(1)
Review Comment:
[P1] Reset observation debt before enabling enforcement
`reserve_with_config()` mutates the shared bucket, but dry-run never sleeps,
so sustained traffic above the configured QPS drives `_remain_tokens`
arbitrarily negative. When an operator turns dry-run off to begin enforcement,
no config callback resets this limiter; the next `add_with_config()` sleeps the
entire historical deficit (for example, 10x the limit for one hour produces
about nine hours of debt). The table strict limiter has the same
mode-transition problem with future deadlines. Please publish/reset or cap
enforcement state on this transition so observation history cannot block live
MetaService RPCs.
##########
be/src/cloud/config.cpp:
##########
@@ -192,6 +192,7 @@
DEFINE_mBool(enable_file_cache_write_cumu_compaction_index_only, "false");
// MS RPC rate limiting config
DEFINE_mBool(enable_ms_rpc_host_level_rate_limit, "false");
+DEFINE_mBool(enable_ms_rpc_host_level_rate_limit_dry_run, "true");
Review Comment:
[P1] Preserve existing enforcement across upgrade
Both new dry-run switches default to `true`, and the runtime paths let
dry-run take precedence over the pre-existing enforcement switches.
Consequently, a deployment that already sets
`enable_ms_rpc_host_level_rate_limit=true` or
`enable_ms_backpressure_handling=true` upgrades without any value for these new
options and silently stops sleeping on both limiters. That removes protection
operators explicitly enabled. Please default dry-run off for compatibility, or
make an enabled enforcement switch take precedence (with four-combination
tests).
##########
be/src/cloud/cloud_throttle_state_machine.cpp:
##########
@@ -137,6 +137,7 @@ std::vector<RpcThrottleAction>
RpcThrottleStateMachine::on_downgrade() {
.rpc_type = rpc_type,
.table_id = table_id,
.qps_limit = old_limit,
+ .reset_reservation = true,
Review Comment:
[P1] Do not double-book outstanding enforced reservations
This reset also runs during actual enforcement, after callers may already
have copied future `wait_until` deadlines and started sleeping outside the
limiter. Setting `_next_allowed_time` to now forgets those outstanding
reservations and gives new callers a second overlapping schedule; for example,
old 1-QPS waiters at t+1/t+2 can collide with a restored 2-QPS schedule at
now/now+0.5/now+1. The aggregate stream then exceeds the supposed strict limit.
Please isolate/reset only dry-run debt, or make outstanding enforced
reservations generation-aware/revalidated before starting a fresh schedule.
##########
be/src/cloud/cloud_ms_backpressure_handler.cpp:
##########
@@ -440,17 +470,27 @@ bool MSBackpressureHandler::on_ms_busy() {
return true;
}
-std::chrono::steady_clock::time_point
MSBackpressureHandler::before_rpc(LoadRelatedRpc rpc_type,
-
int64_t table_id) {
- if (!config::enable_ms_backpressure_handling) {
- return std::chrono::steady_clock::now();
+TableRpcThrottleDecision MSBackpressureHandler::before_rpc(LoadRelatedRpc
rpc_type,
+ int64_t table_id) {
+ const bool dry_run = config::enable_ms_backpressure_handling_dry_run;
+ if (!config::enable_ms_backpressure_handling && !dry_run) {
+ return {.wait_until = std::chrono::steady_clock::now()};
}
- return _throttler->throttle(rpc_type, table_id);
+ auto decision = _throttler->throttle(rpc_type, table_id, dry_run);
+ if (decision.qps_limit > 0) {
+ decision.current_qps = _qps_registry->get_qps(rpc_type, table_id);
Review Comment:
[P2] Defer the QPS read until the log is admitted
`current_qps` is only consumed by the INFO log behind
`should_log_throttle()`, but this call takes the registry shared lock and reads
`bvar::PerSecond` on every request with an active table limit. In the new
default dry-run mode those requests are not slowed, so the full incoming QPS
pays this synchronization/metric cost while at most one request per RPC type
per second logs it. Please move the lookup behind the log-suppression gate.
##########
be/src/cloud/cloud_ms_backpressure_handler.cpp:
##########
@@ -391,7 +419,8 @@ void MSBackpressureHandler::_tick_thread_callback() {
}
void MSBackpressureHandler::_advance_time(int ticks) {
- if (!config::enable_ms_backpressure_handling) {
+ if (!config::enable_ms_backpressure_handling &&
+ !config::enable_ms_backpressure_handling_dry_run) {
Review Comment:
[P1] Prevent the default tick path from overflowing
With dry-run now on by default, this guard advances the coordinator by 1000
every second after the first MS_BUSY. Its elapsed counters are signed `int`s
and continue incrementing even after all upgrade history is gone, so after
about 24.85 days the next addition overflows (undefined behavior). On ordinary
wraparound, `_ticks_since_last_upgrade` becomes negative; a later MS_BUSY
resets only the other counter and then fails the cooldown check, so adaptive
throttling no longer engages. Please use a saturating or 64-bit elapsed counter
(or stop/reset it once history is empty) and cover the boundary.
##########
be/src/cloud/cloud_ms_backpressure_handler.cpp:
##########
@@ -440,17 +470,27 @@ bool MSBackpressureHandler::on_ms_busy() {
return true;
}
-std::chrono::steady_clock::time_point
MSBackpressureHandler::before_rpc(LoadRelatedRpc rpc_type,
-
int64_t table_id) {
- if (!config::enable_ms_backpressure_handling) {
- return std::chrono::steady_clock::now();
+TableRpcThrottleDecision MSBackpressureHandler::before_rpc(LoadRelatedRpc
rpc_type,
+ int64_t table_id) {
+ const bool dry_run = config::enable_ms_backpressure_handling_dry_run;
+ if (!config::enable_ms_backpressure_handling && !dry_run) {
+ return {.wait_until = std::chrono::steady_clock::now()};
}
- return _throttler->throttle(rpc_type, table_id);
+ auto decision = _throttler->throttle(rpc_type, table_id, dry_run);
+ if (decision.qps_limit > 0) {
+ decision.current_qps = _qps_registry->get_qps(rpc_type, table_id);
+ }
+ return decision;
+}
+
+bool MSBackpressureHandler::should_log_throttle(LoadRelatedRpc rpc_type,
int64_t now_us) {
+ return _throttler->should_log(rpc_type, now_us);
}
void MSBackpressureHandler::after_rpc(LoadRelatedRpc rpc_type, int64_t
table_id) {
- if (!config::enable_ms_backpressure_handling) {
+ if (!config::enable_ms_backpressure_handling &&
+ !config::enable_ms_backpressure_handling_dry_run) {
Review Comment:
[P1] Bound the registry before enabling collection by default
Because table dry-run now defaults on, this guard makes every load-related
RPC call `record()`; `get_or_create_counter()` then permanently allocates a
hidden bvar pair for each `(rpc_type, table_id)`. `cleanup_inactive_tables()`
has no caller anywhere in the repository, and the service owns the registry
until BE shutdown, so table-ID churn produces unbounded memory/bvar growth even
when enforcement is off. Please add safe periodic eviction/capping (accounting
for `record()`'s raw-pointer lifetime) or keep this collection opt-in.
##########
be/src/cloud/cloud_ms_backpressure_handler.cpp:
##########
@@ -413,7 +442,8 @@ void MSBackpressureHandler::_advance_time(int ticks) {
bool MSBackpressureHandler::on_ms_busy() {
g_ms_busy_count << 1;
- if (!config::enable_ms_backpressure_handling) {
+ if (!config::enable_ms_backpressure_handling &&
+ !config::enable_ms_backpressure_handling_dry_run) {
Review Comment:
[P1] Do not record floor-to-floor upgrades
This default-on dry-run path calls `on_upgrade()` after every cooldown. Once
a table is already at `floor_qps`, the state machine still emits a SET_LIMIT
and pushes a `{floor, floor}` history record because `old_limit > 0`.
Persistent MS_BUSY therefore grows the vector of map-backed records without
bound; when load recovers, only one no-op record is popped per downgrade
interval, so the first meaningful relaxation is delayed roughly as long as the
preceding overload. Please skip/coalesce unchanged limits and extend the
repeated-floor test to prove history stays bounded and recovery begins promptly.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
