github-actions[bot] commented on code in PR #65974:
URL: https://github.com/apache/doris/pull/65974#discussion_r3638866241
##########
be/src/io/fs/azure_obj_storage_client.cpp:
##########
@@ -186,6 +188,7 @@ struct AzureBatchDeleter {
0 == strcmp(e.ErrorCode.c_str(), BlobNotFound)) {
continue;
}
+
doris::record_object_request_failed(static_cast<int>(e.StatusCode));
Review Comment:
[P2] Inspect all batch subresponses before returning
This returns on the first non-`BlobNotFound` deferred error even though
Azure has already parsed the multipart response and populated every deferred
result. Consequently, a 500 part followed by a 429 never increments
`s3_request_failed_too_many_requests_count`, and multiple 429 parts count only
once; `cloud/src/recycler/azure_obj_client.cpp` has the same early return.
Since the PR exposes a counter for Azure 429 responses, please drain the ready
deferred responses for accounting while preserving the first error for the
method's existing return behavior, and cover mixed/multiple-429 batches.
##########
common/cpp/obj_retry_strategy.cpp:
##########
@@ -65,6 +78,10 @@ std::unique_ptr<Azure::Core::Http::RawResponse>
AzureRetryRecordPolicy::Send(
static_cast<int>(response->GetStatusCode()) < 200) {
if (retry_count > 0) {
object_request_retry_count << 1;
Review Comment:
[P2] Count the 429 that actually triggers the Azure retry
The vendored Azure Core 1.16 pipeline runs `PerRetryPolicies` with retry
count 0 on the first attempt and increments that count only after `RetryPolicy`
schedules the next attempt
([source](https://github.com/Azure/azure-sdk-for-cpp/blob/azure-core_1.16.0/sdk/core/azure-core/src/http/retry_policy.cpp#L106-L188)).
With this gate, `429 -> success` leaves
`s3_request_retry_too_many_requests_count` unchanged; a mixed sequence can also
count a terminal 429 that is not retried. That makes the new metric unreliable
for throttling alerts. Please account at the retry-decision boundary (or mirror
the configured retry options/max-attempt decision here) and add
initial-429-success plus exhaustion/mixed-status coverage.
--
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]
