CAS: parallel delete blobs - #2351
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e0ec8de87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// The entry left the pipeline — drop its in-process condemn-marker confirmation. | ||
| meta_writer->forgetCondemnMarker(entry.ref, entry.token); | ||
| } | ||
| redeleteBlobs(redelete_now, layout, op, new_round, generation, round_work_budget, report, outcomes[shard]); |
There was a problem hiding this comment.
Batch re-deletes across shard boundaries
When cas_gc_shards > 1, this invocation completes and waits for one shard before the loop reaches the next shard, so re-deletes from different shards never overlap. If each shard has fewer than cas_gc_redelete_min_batch_size pending entries, the entire phase remains sequential despite cas_gc_redelete_concurrency > 1; even larger per-shard batches cannot use more concurrency than one shard contains. Schedule the phase's eligible entries before a single wait so the configured fan-out applies across shards.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Default value gc_shards = 1 . Yes, in some cases we could obtain seq execution. But in these cases we don;t have a lot blobs. I suggest skip this impovment now.
| CasOperation job_op = store->openRequests().resume(gen); | ||
| io_results[i] = performRedeleteIo(entries[i], layout, job_op); |
There was a problem hiding this comment.
Account for worker I/O in pending-delete metrics
When parallel re-delete is enabled, performRedeleteIo executes the blob HEAD and conditional DELETE on these pool threads, while GcPhaseTimer snapshots only the round thread's ProfileEvents. As a result, the pending_deletes row in system.cas_gc_log omits the underlying request and retry counters precisely when cas_gc_redelete_concurrency > 1, breaking per-phase operation accounting; collect the worker deltas or expose equivalent explicit phase metrics.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a regression only when the feature is enabled. It is a known limitation that read-ahead also has, and it will be fixed in a separate PR for both places at once.
|
doc combining my + AI review https://gist.github.com/filimonov/a5377e47bf29f92fe12b345d10fe29ad spec + plan to rework (you can pass it to AI agent) https://gist.github.com/filimonov/eb2ef24fbc8a49e2d3cbb43fe43795d8 |
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
077b088 to
3a13b95
Compare
PR #2351 distributed-systems audit
Verdict: two Medium operational defects. No data-safety defect found. 1. GC stop can block for hours during an object-store outage
With the default 5,000-entry budget, a persistent outage can keep one round busy for about 7.8 hours at concurrency 16, or 125 hours at concurrency 1. Fix direction: add cancellation or one shared round deadline, while preserving per-entry accounting for work that already started. 2. A released setting is removed without an alias
This can prevent a node from starting or silently increase GC object-store load during an upgrade. Fix direction: accept both old spellings as deprecated aliases for one release cycle, or at minimum reject both consistently. Review method: static distributed-systems audit of the final PR code and tests. |
PR #2351 CI Verification ReportVerification (2026-09-24)
VerdictNo failure is caused by this PR. CI can be approved. Rebase onto 9 checks fail, plus the aggregate
All failing jobs failed the same way on every attempt. Rerunning them won't turn them green. Still red on this run — categorized
Pre-existing flaky — evidenceRates are runs failed / runs over the last 45 days on 26.6, x86_64. "Branch" means Stateless tsan CAS shard
Alter attach part 3
cas_selects
cas_lightweight_delete_4
Code-review notes (not CI)These don't affect the CI verdict.
Recommendations
|
8661257 to
a2f53e5
Compare
Bench results:
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Added cas_gc_io_concurrency to run blob deletes in the CAS GC pending_deletes phase in parallel.
Documentation entry for user-facing changes
...
CI/CD Options
Exclude tests:
Regression jobs to run: