github-actions[bot] commented on code in PR #64038:
URL: https://github.com/apache/doris/pull/64038#discussion_r3343883759
##########
common/cpp/token_bucket_rate_limiter.h:
##########
@@ -32,11 +32,15 @@ enum class S3RateLimitType : int {
extern std::string to_string(S3RateLimitType type);
extern S3RateLimitType string_to_s3_rate_limit_type(std::string_view value);
-inline auto metric_func_factory(bvar::Adder<int64_t>& ns_bvar,
bvar::Adder<int64_t>& req_num_bvar) {
- return [&](int64_t ns) {
+inline auto metric_func_factory(bvar::Adder<int64_t>& sleep_ns_bvar,
+ bvar::Adder<int64_t>& sleep_count_bvar,
+ bvar::Adder<int64_t>* rejected_count_bvar =
nullptr) {
Review Comment:
Making the rejected counter optional leaves an existing S3 limiter path
without the new metric. `cloud/src/recycler/s3_accessor.cpp` still constructs
its `AccessorRateLimiter` with the old two-argument call (`get_rate_limit_ns`,
`get_rate_limit_exceed_req_num`, etc.), and
`cloud/src/recycler/s3_obj_client.cpp` / `azure_obj_client.cpp` call
`AccessorRateLimiter::instance().rate_limiter(op)->add(1)` directly. As a
result, recycler local limiter rejections still do not increment
`s3_get_rate_limit_rejected_count` / `s3_put_rate_limit_rejected_count`, and
the newly added throttled/rejected logging in `apply_s3_rate_limit()` is
bypassed there. This contradicts the PR's stated metric rename/new rejection
metric behavior for S3 local limiting. Please update the Cloud recycler
accessor path to use the renamed bvars and pass rejection counters, or route it
through a shared helper with the same logging/accounting.
##########
common/cpp/obj_retry_strategy.cpp:
##########
@@ -24,6 +24,16 @@
namespace doris {
bvar::Adder<int64_t> object_request_retry_count("object_request_retry_count");
+bvar::Adder<int64_t> s3_request_retry_too_many_requests_count(
+ "s3_request_retry_too_many_requests_count");
+bvar::Adder<int64_t> s3_request_failed_too_many_requests_count(
+ "s3_request_failed_too_many_requests_count");
+
+void record_object_request_failed(int http_code) {
Review Comment:
The new final-429 failure counter is common, but only the BE object clients
call `record_object_request_failed()`. The Cloud recycler also installs
`S3CustomRetryStrategy` / `AzureRetryRecordPolicy` in
`cloud/src/recycler/s3_accessor.cpp`, so retry 429s from recycler requests are
counted, but final 429 failures in `cloud/src/recycler/s3_obj_client.cpp` and
`azure_obj_client.cpp` just log/return without calling this function. That
makes `s3_request_failed_too_many_requests_count` under-report exactly the
recycler failures that still exhausted retries. Please add the same failure
recording to the Cloud S3/Azure non-success paths (excluding expected not-found
handling) so the retry and failed 429 metrics cover the same object-client
paths.
--
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]
