mrhhsg commented on PR #68032:
URL: https://github.com/apache/doris/pull/68032#issuecomment-5691725660
Thanks for the review. Pushed a follow-up commit addressing it; per finding:
**Fixed**
- `meta_service.cpp` RPC_PREPROCESS: now `(report_spill_stats, get, put)`
with the `get` counter/byte bvars.
- `MetaServiceProxy.getSpillStats`: goes through `executeWithMetrics`
(standard retry / reconnect / MS_TOO_BUSY handling).
- `spill_s3_storage_limit_bytes`: validator rejects negative values;
`spill_s3_max_inflight_upload_bytes`: validator rejects `<= 0`, and the startup
check for `< 2 * s3_write_buffer_size` now matches the runtime path (warning +
degraded one-buffer-at-a-time mode) instead of being fatal.
- **Shared identity** (`spill_file_manager.cpp` / `cloud_meta_mgr.cpp`): you
are right that `cloud_unique_id` is shared by every BE added in one statement.
Both the object root and the stats key now use the FE-assigned `backend_id`
(`spill/{backend_id}/data/{boot_id}/...`,
`stats/{instance}/spill/{backend_id}`); `SpillStatsPB.backend_id` added,
`cloud_unique_id` kept as an informational field. A live BE whose id changes
(DROP + ADD) keeps the id it was bound with and logs a warning; the new id
takes effect at restart.
- Stale reports (`meta_service.cpp` fold): a report whose `boot_id` is older
than the recorded one is rejected (`INVALID_ARGUMENT`) instead of being folded
again; a same-boot report never rolls totals back (`max`). Test covers
old-after-new delivery.
- Startup cleanup (`spill_file_manager.cpp` list): each boot now writes a
small marker `spill/{backend_id}/boots/{boot_id}` (refreshed daily); cleanup
lists only that directory and deletes one old generation per GC round, so the
listing is bounded by the number of boot generations, not by the number of
spill objects.
- `spill_remote_upload_budget.cpp`: `DORIS_CHECK_GE`, no clamp.
- `recycler.cpp`: `spill_objects_expire_time_second <= 0` skips the task
with a warning (test added); config comment documents the "must exceed the
longest query" contract.
- Proto comment: reporting is documented as best-effort (a crash loses at
most one reporting interval; graceful shutdown reports after all tasks are
done).
**Kept as designed (happy to discuss)**
- Request counters (`spill_file_reader.cpp`, `s3_file_writer.cpp`): they
count logical SDK calls, not physical retries. Attempt-level accounting needs a
per-request hook inside the SDK retry strategy (`S3CustomRetryStrategy` is
process-global with no request context); the byte counters, which are the
primary billing input, are exact. Documented as the counting unit; can be a
follow-up.
- Durable per-upload checkpoint (`spill_file_manager.cpp` counters): writing
to meta-service on every upload is not worth the cost; the loss bound is one
reporting interval and only on crash.
- Capacity release on failed delete (`spill_file.cpp`): same behaviour as
the local spill path; retained objects are reclaimed by the query-directory
retry and the recycler.
- Incomplete multipart uploads (`spill_file_writer.cpp` abort): failed
uploads are aborted; residue of a crashed process is invisible to any prefix
listing, and neither the BE object client nor the recycler has
`ListMultipartUploads`. The bucket-level `AbortIncompleteMultipartUpload` rule
is the intended safety net (now stated in the config comments).
- Stats cardinality (`get_spill_stats`): with `backend_id` the family grows
with "backends ever created" rather than with restarts. Folding retired
backends on DROP BACKEND is a reasonable follow-up.
- Recycler liveness fence: the TTL is documented as "larger than the longest
query the cluster allows"; a query holding spill for more than 7 days is
outside what this PR targets.
--
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]
