[Store] Notify master on disk eviction to fix stale metadata #1549
[Store] Notify master on disk eviction to fix stale metadata #1549duhaode520 wants to merge 4 commits intokvcache-ai:mainfrom
Conversation
- FileRecord gains a key field to track evicted object keys - StoreObject returns evicted key list; EvictFile returns FileRecord - EnsureDiskSpace collects evicted keys through FIFO eviction loop - MasterService::EvictDiskReplica(client_id, key, replica_type) with DISK vs LOCAL_DISK distinction and per-client filtering - Wire through RPC layer: WrappedMasterService + MasterClient - Unit tests for StorageBackend eviction and MasterService EvictDiskReplica
- PutToLocalFile passes key to StoreObject, calls EvictDiskReplica for each evicted key with ReplicaType::DISK - Client::EvictDiskReplica public method forwarding to MasterClient - FileStorage::OffloadObjects creates eviction_handler lambda calling EvictDiskReplica with ReplicaType::LOCAL_DISK, passes to BatchOffload - StorageBackendAdaptor::BatchOffload invokes eviction_handler per evicted key from StoreObject - Unit test for eviction_handler callback in BatchOffload
- Extend InProcMasterConfig with root_fs_dir, enable_disk_eviction, and quota_bytes to allow tests to configure disk replica behavior - Add EvictionNotificationTest fixture with small quota (3KB) to trigger FIFO eviction after 3 x 1KB puts - Verify evicted key's DISK replica is removed from master metadata via EvictDiskReplica RPC, and non-evicted keys retain their DISK replicas
duhaode520
requested review from
XucSh,
YiXR,
stmatengss and
ykwd
as code owners
Summary of ChangesHello @duhaode520, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where the master service's metadata could become stale after local disk eviction, leading to data access errors. The changes introduce a robust notification system, ensuring that the master is always aware of locally evicted disk replicas. This improves data consistency and reliability by actively synchronizing the master's view of stored objects with the actual state of local storage. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of stale metadata on the master after local disk eviction. The solution is well-designed, introducing a new EvictDiskReplica RPC and correctly propagating eviction information from the StorageBackend up to the client service to trigger master notification. The changes are comprehensive, covering both the Put and BatchOffload paths. The addition of unit and integration tests is commendable and significantly improves confidence in the fix. I've identified a couple of areas for improvement regarding code duplication and test clarity, which are detailed in the review comments.
| TEST_F(StorageBackendTest, AdaptorBatchOffload_EvictionHandlerCalled) { | ||
| // Test that the eviction_handler callback in BatchOffload is correctly | ||
| // invoked when the underlying StorageBackend evicts files during | ||
| // StoreObject. We use a direct StorageBackend with a small quota to | ||
| // guarantee eviction, then verify via StorageBackendAdaptor that | ||
| // the handler fires. | ||
| // | ||
| // Since StorageBackendAdaptor::Init doesn't forward quota to the | ||
| // underlying StorageBackend, we test at the StoreObject level (already | ||
| // covered by StoreObjectReturnsEvictedKeys) and verify the BatchOffload | ||
| // handler wiring here with a mock-like capture. | ||
|
|
||
| std::string test_dir = data_path + "/eviction_handler_test"; | ||
| std::filesystem::create_directories(test_dir); | ||
|
|
||
| // Create backend with small quota (3072 bytes = room for ~3 files of 1024) | ||
| StorageBackend backend(test_dir, "", true); | ||
| auto init_result = backend.Init(3072); | ||
| ASSERT_TRUE(init_result.has_value()); | ||
|
|
||
| // Pre-fill with keyed files | ||
| std::string data(1024, 'A'); | ||
| auto r1 = backend.StoreObject(test_dir + "/f1", data, "key_1"); | ||
| ASSERT_TRUE(r1.has_value()); | ||
| auto r2 = backend.StoreObject(test_dir + "/f2", data, "key_2"); | ||
| ASSERT_TRUE(r2.has_value()); | ||
| auto r3 = backend.StoreObject(test_dir + "/f3", data, "key_3"); | ||
| ASSERT_TRUE(r3.has_value()); | ||
|
|
||
| // Now store one more, which should evict key_1 | ||
| std::vector<std::string> evicted_keys; | ||
| auto r4 = backend.StoreObject(test_dir + "/f4", data, "key_4"); | ||
| ASSERT_TRUE(r4.has_value()); | ||
| for (const auto& ek : r4.value()) { | ||
| evicted_keys.push_back(ek); | ||
| } | ||
|
|
||
| EXPECT_FALSE(evicted_keys.empty()) | ||
| << "Should have evicted at least one key"; | ||
| EXPECT_EQ(evicted_keys[0], "key_1") | ||
| << "FIFO eviction should evict key_1 first"; | ||
| } |
There was a problem hiding this comment.
This test case, AdaptorBatchOffload_EvictionHandlerCalled, appears to be redundant and its name is misleading. The implementation directly tests StorageBackend::StoreObject, which is already covered by StoreObjectReturnsEvictedKeys. It does not test StorageBackendAdaptor::BatchOffload or the eviction_handler callback as the name and comment suggest.
To improve test clarity and avoid redundancy, consider one of the following:
- Remove this test: Since its core logic is already tested, removing it would clean up the test suite.
- Refactor the test: If the goal is to test the
eviction_handlerwiring inStorageBackendAdaptor, the test should be refactored to instantiate aStorageBackendAdaptorand provide a mockeviction_handlerto verify it's called with the correct evicted keys. This might require some adjustments to theStorageBackendAdaptorto allow injecting aStorageBackendwith a specific quota for testing purposes.
As it stands, the test is confusing and duplicates existing test coverage.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
2 participants
Description
When
StorageBackendevicts local disk files via FIFO eviction (EnsureDiskSpace→EvictFile) to make space for new writes, the master is not notified. This causes staleDiskDescriptor/LocalDiskDescriptormetadata to persist in the master, pointing to files that no longer exist on disk. SubsequentGetrequests are routed to non-existent files, causingmetadata not founderrors.This issue was identified during PR #1028 review (comment by @ykwd: "Before removing a file, the master should be notified"). Related issues: #1078.
Problem
Solution
FileRecordgains akeyfield — tracks which object key each file belongs to, so eviction can report the affected key.StoreObject/EnsureDiskSpacereturn evicted keys — callers receive astd::vector<std::string>of keys whose files were evicted during the write.New
EvictDiskReplicaRPC — a dedicated master-side operation that removes DISK or LOCAL_DISK replicas for a key without affecting memory replicas. UnlikePutRevoke(PROCESSING-only) orRemove(deletes entire key), this works on COMPLETE replicas and preserves memory replicas.Client-side notification in both write paths:
PutToLocalFile: afterStoreObject, iterates evicted keys and callsEvictDiskReplica(key, DISK)BatchOffload: neweviction_handlercallback parameter;FileStoragepasses a handler callingEvictDiskReplica(key, LOCAL_DISK)Scope
EnsureDiskSpaceduringStoreObject(bothPutToLocalFileandBatchOffloadpaths).InitQuotaEvict(startup-time eviction) — can rely onScanMetato re-sync with master on restart.Type of Change
How Has This Been Tested?
Unit Tests (storage_backend_test.cpp)
StoreObjectReturnsEvictedKeys: fills 2KB quota with 2×1KB files, writes 3rd → verifies evicted key returned in FIFO orderStoreObjectEvictionWithEmptyKey: writes without key → verifies empty eviction list (backward compat)AdaptorBatchOffload_EvictionHandlerCalled: 3KB quota, writes 4×1KB files → verifieseviction_handlercalled with correct evicted keyUnit Tests (master_service_ssd_test.cpp)
EvictDiskReplica_RemovesDiskReplica: creates MEMORY+DISK replicas, evicts DISK → verifies MEMORY preservedEvictDiskReplica_NonExistentKeyReturnsError: verifiesOBJECT_NOT_FOUNDEvictDiskReplica_InvalidReplicaTypeReturnsError: verifiesINVALID_PARAMSfor MEMORY typeIntegration Tests (client_integration_test.cpp)
EvictionNotifiesMaster: starts InProcMaster with 3KB disk quota, puts 3×1KB keys (fills quota), puts 4th key → verifies key_0's DISK replica removed from master metadata while keys 1-3 retain their replicasAll 3 test suites pass: storage_backend_test (49), master_service_ssd_test (10), client_integration_test (11).
Checklist
./scripts/code_format.shbefore submitting.