Copilot commented on code in PR #67421:
URL: https://github.com/apache/doris/pull/67421#discussion_r3910489676
##########
fe/fe-common/src/main/java/org/apache/doris/common/Config.java:
##########
@@ -3890,7 +3890,12 @@ public static int metaServiceRpcRetryTimes() {
@ConfField(mutable = true, description = {
"Whether to enable QPS rate limit for RPC requests to meta
service."})
- public static boolean meta_service_rpc_rate_limit_enabled = false;
+ public static boolean meta_service_rpc_rate_limit_enabled = true;
+
Review Comment:
`meta_service_rpc_rate_limit_enabled` default was changed to `true`, which
enables the rate limiter path by default for all deployments. This is a
behavioral/operational change (added overhead + new metrics/logging) and
doesn’t seem required for introducing dry-run mode; consider keeping the
default `false` and letting operators opt in explicitly.
##########
fe/fe-common/src/main/java/org/apache/doris/common/Config.java:
##########
@@ -3890,7 +3890,12 @@ public static int metaServiceRpcRetryTimes() {
@ConfField(mutable = true, description = {
"Whether to enable QPS rate limit for RPC requests to meta
service."})
- public static boolean meta_service_rpc_rate_limit_enabled = false;
+ public static boolean meta_service_rpc_rate_limit_enabled = true;
+
+ @ConfField(mutable = true, description = {
+ "Whether to only evaluate and report meta service RPC rate limits
without waiting or rejecting requests. "
+ + "This takes effect only when meta service RPC rate
limiting is enabled."})
+ public static boolean meta_service_rpc_rate_limit_dry_run = true;
Review Comment:
`meta_service_rpc_rate_limit_dry_run` default is set to `true`, which means
rate limiting will run in dry-run by default as soon as the feature is present.
If the intent is to preserve existing behavior unless explicitly configured,
default this to `false` so enabling dry-run remains an explicit operator action
(especially on a stable branch backport).
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/rpc/MetaServiceRpcRateLimiter.java:
##########
@@ -68,7 +68,22 @@ long acquire(String methodName, int permits) throws
RpcException {
// Resilience4j returns negative when the estimated wait exceeds the
configured timeout.
// Otherwise the returned wait time is within
meta_service_rpc_rate_limit_wait_timeout_ms.
long nanosToWait =
holder.rateLimiter.reservePermission(permitsToAcquire);
+ boolean dryRun = Config.meta_service_rpc_rate_limit_dry_run;
if (nanosToWait < 0) {
+ if (MetricRepo.isInit) {
+ CloudMetrics.META_SERVICE_RPC_ALL_RATE_LIMITED.increase(1L);
+
CloudMetrics.META_SERVICE_RPC_RATE_LIMITED.getOrAdd(methodName).increase(1L);
+ }
Review Comment:
Metrics update here assumes `CloudMetrics.META_SERVICE_RPC_*` are
initialized, but `CloudMetrics.init()` is a no-op in non-cloud mode, leaving
these fields null while `MetricRepo.isInit` can still be true. Guarding only by
`MetricRepo.isInit` can lead to NPEs if this limiter is ever exercised when
cloud metrics aren’t initialized (e.g., tests, misconfiguration). Add null
checks (or a `Config.isCloudMode()` guard) before dereferencing these metrics.
This issue also appears in the following locations of the same file:
- line 96
- line 124
--
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]
