[manager] make CacheReclaimer deletion asynchronous and prevent over-eviction#234
[manager] make CacheReclaimer deletion asynchronous and prevent over-eviction#234shaohuaxi wants to merge 1 commit into
Conversation
0ec6ceb to
b800f4c
Compare
oldsharp
left a comment
There was a problem hiding this comment.
A high-quality, well-tested change with a clear design rationale.
Two questions to concern in this round of review:
- about the
SubmitAsync()'s accepting task type; - about the integration test implementation.
See the inline comments for more detail.
|
|
||
| std::future<PlanExecuteResult> Submit(const CacheMetaDelRequest &task); | ||
| std::future<PlanExecuteResult> Submit(const CacheLocationDelRequest &task); | ||
| AsyncDeleteSubmitResult SubmitAsync(const CacheLocationDelRequest &task); |
There was a problem hiding this comment.
Should SubmitAsync accept CacheMetaDelRequest too?
No such use case under the current code base, but the case is reasonable.
CacheMetaDelRequest has the same problem the V1 async design was built to solve: Get/CAS/Sync may hit remote Redis and block the caller thread. And by making SubmitAsync accept CacheMetaDelRequest type task can let it align better to the other two Submit variants.
There was a problem hiding this comment.
Thanks for reviewing.
Addressed by adding SubmitAsync(const CacheMetaDelRequest&) and reusing the same admission, delayed physical deletion, shutdown cancellation, and exactly-once Future completion pipeline as CacheLocationDelRequest. Existing synchronous callers remain unchanged. Unit tests cover worker execution, CLS_DELETING idempotency, failures, cancellation, and enqueue rejection.
| 3. Future 超过 deadline 后,验证 credit 回弹、pending 保留且提交量受硬上限约束。 | ||
| 4. 某个 Group × Type 达到上限后,验证其他未饱和 Type 在 Executor 可用时仍可提交。 | ||
| 5. 没有有效 victim、全部 pending 和入队拒绝时,验证 ReclaimCron 不会零间隔空转。 | ||
| 6. 同一 Group 多 Instance 时,验证一轮内水位满足后停止继续提交。 |
There was a problem hiding this comment.
Strong integration test verification design; would be better to add corresponding test implementations (similar to what #161 did in integration_test/reclaimer/reclaiming_test.py).
There was a problem hiding this comment.
Addressed. Added Reclaimer integration tests covering byte/key-count credit under large delete delay, multiple Instances in the same Group, Future-state metric cleanup, and no-progress backoff. The multi-Instance case now uses metric polling instead of fixed sleep and passed 10 consecutive runs on the 193 test environment.
b800f4c to
e0f357e
Compare
Background
Before this change, CacheReclaimer submitted a Location deletion by synchronously performing metadata Get, CAS, and Sync. These operations may access a remote metadata backend such as Redis, so a slow metadata operation could block the Reclaimer cron thread and delay reclamation work for every Instance Group handled by that loop.
Deletion may also be delayed by
delay_before_delete_ms. During that interval, official storage usage and key count have not decreased yet, so subsequent cron iterations continue to observe an exceeded watermark and select more victims. The accumulated submissions can evict substantially more cache data than required. Making admission asynchronous also creates an accepted-before-CAS window in which a Location still appears selectable, so in-flight work must be accounted for and deduplicated immediately after acceptance.Summary
Submitinterface.SchedulePlanExecutorworkers; delayed waits use the timer queue and do not occupy a worker.Key behavior
SubmitAsyncreturnsaccepted=falsewith no valid Future when the initial enqueue is rejected; Reclaimer state is created only after acceptance.delay_before_delete_msstarts after metadata Sync succeeds.instance_id, block key, and location id, preserving strict Instance isolation.VCNS_HF3FSnormalized toHF3FS; all effective-watermark subtraction is saturating.Validation
All compilation and tests were run in a dedicated Linux container.
Results:
SchedulePlanExecutorTest: 16 tests, target passed 10/10 runs.CacheReclaimerTest: 75 tests, target passed 10/10 runs.ServerConfigTest: 3 tests, target passed 10/10 runs.//kv_cache_manager:kv_cache_manager_bin: built successfully with--jobs=1.git diff --check: passed.