[manager] move EventReport cleanup into background GC - #281
Conversation
shaohuaxi
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8770c29720
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![qoderai[bot]](https://avatars.githubusercontent.com/in/1396158?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Reviewed PR #281 (EventReport active cleanup integrated into background scan GC). The design document (docs/design/event_report_background_gc.md) is thorough and well-structured. The intent model, three-state lifecycle lease, per-instance cursor pass, fair scheduling, and maintenance metadata API contracts are clearly documented and faithfully implemented.
Key strengths
- Intent merge/replace semantics correctly handle event storms (multiple Snapshots/HOST_DOWN collapse into per-Instance passes).
- Maintenance no-touch reads (
ApplyToEntryNoTouch,GetForOneKeyForMaintenance) prevent LRU/access-time pollution — the two-phaseScanLocationsForMaintenancerefactor inmeta_local_backend.ccis a clean fix for the prior shard-mutex-during-item-read concern. - Accounting safety: prior mutation flags are carried across Sync failures (
meta_searcher.cclines 2001-2007) to prevent double-counting, andCommitMaintenanceDeleteAccountingonly runs after all hard errors converge. - Shutdown ordering (RequestStop → ClearCallbacks → Join → Reset) and fail-closed restart semantics (no persisted generation, intent/cursor discarded) are sound.
- Test coverage is comprehensive across intent lifecycle, lease states, maintenance delete, metrics, and E2E.
Findings (3 inline comments)
- kBusy blocks entire batch (
cache_garbage_collector.cc:1263): One busy reporter fails the whole action batch, preventing progress for unrelated reporters. Considercontinue+out_revisit_required=true. - kStale early return skips remaining targets (
cache_garbage_collector.cc:1275): Inconsistent withBeginEventReportPass(line 973 usescontinue). Remaining targets are deferred to a full pass rescan unnecessarily. - String-based metric lookups on hot path (
cache_garbage_collector.cc:1506): Event report metrics useGetCounter("cache_gc.event_report_*")per call, unlike the regular GC path which uses cachedMETRICS_macros.
All findings are low-to-medium severity — the implementation is functionally correct and the issues are about efficiency and consistency.
🤖 Generated by Qoder
e2d6708 to
8f48e72
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f48e72957
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
5a0246e to
0939176
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09391763ac
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
9a20dbb to
67cfab2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67cfab2844
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| out_results.clear(); | ||
| prior_results = nullptr; |
There was a problem hiding this comment.
When cached metadata is enabled and the persistent deletion succeeds but the hot-layer deletion returns a hard error, this settlement accounts the persistent mutation and then clears all record of it. The subsequent retry can successfully remove the same logical location from hot cache, causing CommitMaintenanceDeleteAccounting() to subtract its storage usage—and, for a whole-key deletion, its key count—a second time. Carry an already-accounted layer marker into the retry or defer accounting until both layers converge.
Useful? React with 👍 / 👎.
67cfab2 to
879020a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 879020af84
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const bool persistent_safe = layer_has_no_unauthorized_locations(i, persistent_id_ecs, persistent_location_ids); | ||
| const bool hot_safe = !cache_backend_ || layer_has_no_unauthorized_locations(i, hot_id_ecs, hot_location_ids); | ||
| delete_whole_keys[i] = persistent_safe && hot_safe; |
There was a problem hiding this comment.
When either layer's GetLocationIdsForMaintenance() returns a timeout or other error while the targeted GetLocationsForMaintenance() succeeds, these checks merely disable whole-key deletion; the target is still authorized and deleted through the location-only plan below. If it was the last location, this leaves an empty metadata key without any reclaimed_*_key flag, so MetaIndexer never decrements its key count, and later scans cannot rediscover a location to repair the accounting. Treat an ID-enumeration error as a hard per-key failure that retains the intent for retry.
Useful? React with 👍 / 👎.
1 participant
Background
EventReport currently schedules an instance-wide metadata scan after every successful Snapshot and HOST_DOWN/liveness cleanup. Repeated events can therefore multiply full scans and compete with deletion and migration work in the shared executor.
This PR builds on #244 and moves those cleanup paths into the resident background GC. EventReport events register or advance cleanup intents; GC merges compatible intents and reconciles them with bounded per-instance scans.
Changes
Correctness boundaries
Dependency
This is the second layer of a stacked change and depends on #244.
Stack created with GitHub Stacks CLI • Give Feedback 💬