Doris-Breakwater commented on issue #70504:
URL: https://github.com/apache/doris/issues/70504#issuecomment-6818338714

   Breakwater-GitHub-Analysis-Slot: slot_d03c72f6404d
   
   **Assessment:** The batch-wide retention mechanism is confirmed by source 
inspection of both tag `4.1.4` (`ad35a140c7fd`) and the reported master 
revision `e125ea85867`. This is actionable as a cloud recycler 
progress/retry-amplification bug. The available issue snapshot has no labels; 
cloud/recycler and bug would be appropriate triage categories. Please open the 
proposed PR with its tests.
   
   **Verified mechanism**
   
   - In [`recycle_rowsets` at the reported 
revision](https://github.com/apache/doris/blob/e125ea858678fd2aea5ebf179359650aa77fabb6/cloud/src/recycler/recycler.cpp#L6018),
 a nonzero aggregate `delete_rowset_data` result returns before bitmap cleanup, 
recycle-key removal, and the `num_recycled` increment. Successfully deleted 
rowsets in that worker therefore retain their recycle keys. The [4.1.4 
worker](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/cloud/src/recycler/recycler.cpp#L5697)
 has the same gate. Both the normal temporary-rowset worker and its 
abort/recheck helper have analogous gates.
   - 
[`delete_rowset_data`](https://github.com/apache/doris/blob/e125ea858678fd2aea5ebf179359650aa77fabb6/cloud/src/recycler/recycler.cpp#L4556)
 combines file paths by storage resource and reduces deletion/preparation 
failures to one status. The accessor exposes only a status. On master, 
[`S3ObjStorageClient::delete_objects`](https://github.com/apache/doris/blob/e125ea858678fd2aea5ebf179359650aa77fabb6/common/cpp/obj-client/s3_obj_storage_client.cpp#L409)
 returns at the first failed request or response containing per-key errors; 
previously completed chunks and unsent chunks are not distinguished for 
callers. Executor cancellation can also leave work unexecuted.
   - The recycler explicitly selects `retry_slow_down=false`, and the [retry 
strategy](https://github.com/apache/doris/blob/e125ea858678fd2aea5ebf179359650aa77fabb6/common/cpp/obj_retry_strategy.cpp#L43)
 suppresses SDK retries specifically for `SlowDown` with HTTP 503. This agrees 
with [#65508](https://github.com/apache/doris/pull/65508); other retryable 
errors retain their retry policy. 
[#63377](https://github.com/apache/doris/pull/63377) establishes the 
successful-tablet-only metadata cleanup pattern, but does not supply per-rowset 
results here.
   
   Two details matter for implementation and reproduction:
   
   1. The formal-rowset worker is submitted per KV scan page. 
`recycler_max_tasks_per_batch` controls recursive object-delete task grouping, 
rather than directly setting this worker's rowset count. S3's 1,000-object 
request limit is a separate boundary. A rowset can span multiple requests. 
Explicit-key deletion reissues deletes on retry; prefix deletion additionally 
lists objects.
   2. The [4.1.4 cloud S3 
client](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/cloud/src/recycler/s3_obj_client.cpp#L261)
 checks request-level failure but does not inspect successful `DeleteObjects` 
responses for per-key `Errors`. Master does inspect them. A release fix must 
account for this difference.
   
   **What remains unverified:** I have not run the reported unit test or 
reproduced sustained throttling. Repeated work follows from the code, but 
indefinite starvation or a completely stalled backlog is conditional on 
failures continuing in the affected batches. Successful independent batches can 
still progress. Counter differences alone cannot establish S3 throttling: 
bitmap/KV failures, preparation failures, and mark-before-delete phases can 
also retain keys.
   
   Please attach the PR/test and exact deployed Meta Service/recycler build 
SHA; sanitized consecutive-round logs showing `num_expired`/`num_recycled`, 
delete HTTP status/error code/request ID, and any bitmap/KV errors; effective 
limiter, concurrency, and mark/abort settings; and the injection point, 
object-to-rowset mapping, and before/after KV assertions for the 10-rowset 
case. Include whether versioned recycling is active, since `recycle_rowsets` 
can dispatch to another path. No query Profile is needed for this deletion-path 
diagnosis.
   
   **Suggested PR acceptance criteria:** Preserve explicit object-to-rowset 
ownership and report completed rowsets conservatively. Per-key errors, 
uncertain request outcomes, unsent chunks, and cancelled tasks must remain 
retryable. Remove a rowset's recycle key only after all its required 
data/refcount and bitmap work succeeds; preserve mark/abort/recheck ordering 
and packed/shared-file safety. Advance recycling counters after successful KV 
cleanup. Cover both temporary-rowset paths, cross-request rowsets, mixed 
resources, HTTP-200 per-key errors, whole-request failures, cancellation, 
bitmap/KV failures, and a second round proving completed rowsets are not 
resent. For the stated 10-rowset case, eight keys should disappear only if 
those eight rowsets completed all required work.
   
   Review adaptive pacing separately from partial-success accounting. A bounded 
shared cooldown with jitter is reasonable to evaluate, but should preserve the 
deliberate recycler fail-fast SDK policy, avoid holding locks while waiting, 
and test concurrent callers and recovery.
   
   For interim mitigation, the version-matched code confirms DELETE uses the 
PUT limiter. Enable it with a measured rate **and a bounded 
`s3_put_bucket_tokens` burst**, verify the effective limiter is reset/applied, 
and observe deletion throughput versus incoming expired rowsets. The default 
burst is enormous, so lowering only `s3_put_token_per_second` may not provide 
prompt pacing. This reduces request pressure; it does not repair 
partial-success accounting or guarantee a bucket-wide limit across recycler 
processes.
   


-- 
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]

Reply via email to