Repository navigation
Conversation
b1891db to
8c59b5a
Compare
Extract ObjectMetadata and the shared per-object runtime types out of MasterService into independent headers, and introduce the tenant-scoped container (mooncake::tenant::TenantStore) plus the copy-on-write TenantDirectory that owns one store per tenant. The per-object unit now carries its own metadata (owned by the entry) and a per-object mutex, so unrelated objects never contend on one shard lock. Fix has_invalid_mem_handle() to guard a null buffer (freed before full teardown) instead of dereferencing it.
Flip MasterService from sharded metadata maps to a TenantDirectory that owns one TenantState per tenant, and give each object its own ObjectEntry with a per-object ObjectEntry::mutex. Concurrent point ops on unrelated objects no longer share a per-tenant or per-shard lock; the mutation boundary is one object. Routing is decoupled from group membership: groups become a thin membership table plus one shared Lease, single-sourced in each tenant's object route and consulted only at eviction (all-or-none). The route is a single lock (an unordered_map key -> strong ObjectEntry handle); the per-object lock and the route lock are never held together (released before any route mutation), and group wiring is ordered and rolled back on publish failure so a pinned entry never observes an un-wired grouped member. lease_ access is uniformly guarded by ObjectMetadata's own lock. Gate tests are adjusted for the object-level boundary and a deterministic snapshot-barrier hook; the test-only route lock is reached only via a MasterService friend seam. MasterService teardown serializes RemoveAll under the exclusive snapshot lock.
ac03eaa to
00aa883
Compare
Fold the remaining per-object-locking follow-ups on top of the two core feature commits (per-tenant module foundation and object-level locking in MasterService): - rename MetadataShardAccessor -> TenantStateAccessor - retire shard-era naming, scan-budget param, and bucket-shrink machinery - drop production-dead accessors, redundant IsGrouped, and tautology tests - re-pin the Create() winner on a lost insert; fix include hygiene - relocate the tenant module headers (tenant_directory.h, object_entry_types, tenant_id.h) into include/tenant/ - adjust the DFS eviction valid-replica guard and drop the shard-era RemoveAll race tests (the tenant-first RemoveAll serializes the scan under an exclusive snapshot lock, so that race window no longer exists) All mooncake-store gate suites pass.
ef67a6a to
0efd9ac
Compare
… cleanup Carry out the shard-era naming cleanup that the earlier rebase left in place (the pure-rename commit was dropped as dead-cosmetic against the migrated base): - Rename the MasterService accessor plumbing: GetShard() -> GetTenantAccessor(), shard_guard_ -> tenant_guard_, and every TenantStateAccessorRW local/parameter named 'shard' -> 'tenant_accessor'. - Drop the dead SetRemoveAllShardHookForTesting / kv_remove_all_shard_hook_ test hook: the tenant-first RemoveAll serializes the whole scan under an exclusive snapshot lock, so the per-shard-lock-release hook it was built around no longer exists and has no call site. - Rewrite the EvictGroupOrObject header comment (the AB/BA cross-shard deadlock text) to the per-object lock + per-tenant-object_route model. - Refresh stale comments that still described metadata shards / per-shard task maps / shard lock ordering; keep the genuinely-shard-based ones (getShardIndex compatibility, snapshot 'shards' data format, sharded quota table).
…name
The accessor rename rewrote user-facing snapshot 'shards'-format error
messages ('expected binary data for shard', 'Failed to unpack shard data',
'Invalid shard format') to 'tenant_accessor'. These describe the on-wire
snapshot 'shards' key, not the tenant accessor, so restore them.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Implements tenant-first, object-level metadata locking for Mooncake Store by replacing shard-wide serialization with per-object locks and introducing per-tenant routing/group state under a copy-on-write tenant directory.
Changes:
- Added tenant module primitives:
TenantDirectory(COW registry),TenantStore(object route + flat group membership), andObjectEntry(per-key lock + task state). - Refactored
MasterServicemetadata/task tracking to live onObjectEntryand tenant-scoped containers (removing metadata shards / global group table usage). - Updated and added tests/benchmarks to validate tenant routing, group lease semantics, and object-entry behavior.
Reviewed changes
Copilot reviewed 29 out of 31 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| mooncake-store/tests/tenant_store_test.cpp | Adds unit tests for tenant object route + flat group membership and leases |
| mooncake-store/tests/tenant_id_test.cpp | Updates include path for relocated tenant headers |
| mooncake-store/tests/tenant_directory_test.cpp | Adds unit tests for COW tenant directory behavior and concurrency |
| mooncake-store/tests/promotion_on_hit_test.cpp | Updates promotion task assertions to pull state from ObjectEntry |
| mooncake-store/tests/object_entry_test.cpp | Adds unit tests for per-object metadata ownership and locking API |
| mooncake-store/tests/master_service_test_fixture.h | Refactors test helpers to tenant-first routing + adds accessor regression helper |
| mooncake-store/tests/master_service_test.cpp | Drops obsolete shard-lock race tests; adds regression for accessor create re-pin |
| mooncake-store/tests/master_service_tenant_quota_test.cpp | Updates quota tests to use tenant directory handles instead of shard accessors |
| mooncake-store/tests/master_service_processing_key_double_erase_test.cpp | Simplifies keepalive key selection under tenant-only container model |
| mooncake-store/tests/master_service/dsl/scenario.cpp | Updates DSL scenario mutations to use per-tenant lookup and per-object locks |
| mooncake-store/tests/ha/snapshot/snapshot_child_process_test.cpp | Updates snapshot tests to tenant directory and route-based lookups |
| mooncake-store/tests/ha/snapshot/master_snapshot_codec_test.cpp | Updates tenant header include path |
| mooncake-store/tests/ha/master_service_ha_test.cpp | Updates HA tests to gate via tenant route lock + snapshot barrier hooks |
| mooncake-store/tests/file_storage_test.cpp | Updates tenant header include path |
| mooncake-store/tests/dynamic_replication_test.cpp | Updates dynamic replication tests for entry-based task state |
| mooncake-store/tests/CMakeLists.txt | Registers new unit test targets for tenant module types |
| mooncake-store/src/ha/snapshot/local_ssd_codec.cpp | Updates tenant header include path |
| mooncake-store/include/types.h | Updates tenant header include path |
| mooncake-store/include/tenant_quota.h | Updates tenant header include path |
| mooncake-store/include/tenant/tenant_store.h | Introduces per-tenant object route + flat group membership + DR lease table |
| mooncake-store/include/tenant/tenant_directory.h | Introduces lock-free read / COW write tenant registry |
| mooncake-store/include/tenant/object_entry_types.h | Extracts shared per-key runtime task types/enums from MasterService |
| mooncake-store/include/tenant/object_entry.h | Adds ObjectEntry with per-object shared_mutex and metadata pointer ownership |
| mooncake-store/include/replica.h | Fixes null-deref risk in has_invalid_mem_handle() |
| mooncake-store/include/object_metadata.h | Extracts ObjectMetadata out of MasterService for tenant module reuse |
| mooncake-store/include/master_service.h | Refactors MasterService to tenant-first routing + per-object locking + new test hooks |
| mooncake-store/include/local_ssd/manager.h | Updates tenant header include path |
| mooncake-store/include/kv_event/kv_event_publisher.h | Updates tenant header include path |
| mooncake-store/benchmarks/batch_evict_bench.cpp | Updates eviction benchmark to walk tenant directory + entries |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Apply the exact reflow the LLVM 22 format check requires: rewrap the lock-order comment, drop a stray blank line, and re-wrap the duration_cast in EraseExpiredDynamicReplicationLeases.
Every scope now uses a single tenant_state reference; the parallel tenant_accessor aliases (a leftover from the removed shard accessor type) are gone, and the two helpers that aliased their own parameter take tenant_state directly.
Rename the one remaining bare tenant reference to tenant_state, rename the promotion retry noop test to what it actually exercises, fix a stale re-Pin comment, and drop the tautological re-route test that only asserted freshly created objects are reachable.
LeaseForTest moves to TenantCatalog and becomes private on GroupIndex (friended), so test introspection goes through the catalog boundary instead of reaching into its group index member.
GroupIndex, ObjectIndex, quota_account and disk_object_count become private; external code now goes through intention-revealing catalog methods (GroupMembers, the dynamic-replication lease surface, BindQuotaAccount/BoundQuotaAccount, AccountRestoredDiskObject, plus the existing route accessors). Test introspection (LeaseForTest, HasDynamicReplicationLeaseForKeyForTest, LockRouteForTesting) is forwarded through the catalog as well, the group-contention benchmark uses a standalone GroupIndex, and the whitebox test that hand-built a disconnected route/membership state is dropped — that state is unconstructible through the publication API by design.
Aionw
left a comment
Aionw
left a comment
There was a problem hiding this comment.
Re-verified on 3f530134. Most of the previous review is fixed — I re-ran the deterministic probes plus the repo suites.
Fixed (probes now pass):
UpsertStart,FinalizeRemovedReplicasAfterDurable,BatchRemoveclaim teardown under the entry lock (R2a/R2b/R2c).Erase()hands its held lock toEraseMetadataLocked; a Get arriving during the teardown handoff is no longer served (R3).- The reaper /
ClearStaleHandles/ snapshot cleanup keep pinned handles instead of re-resolving keys (R1, synchronous paths). Create/AllocateAndInsertMetadatalock the envelope before publishing (R5 empty-envelope race).GroupIndex::AddMembermaterializes + registers + returns the lease atomically (R6).- Repo suites:
master_service_test57/57,master_service_group_test11/11, catalog tests 20/20.
Still open (probes fail):
- The HA/oplog deferred finalizers are still key-addressed and act on whatever entry is current (
FinalizeMetadataEraseAfterDurable,FinalizeExpiredProcessingReplicasAfterDurable). - Manual read paths (
BatchExistKey,GetReplicaListByRegex,GetReplicaListForAdmin,GetAllKeys) do not checkis_torn_down. Create()'s insert-failure fallback can adopt a torn-down entry, soAddReplicacan acknowledge a registration for a key that is already gone.- (doc) The new lock-order invariant comment is contradicted by the entry -> route nesting in
Create/AllocateAndInsertMetadata.
Details in the inline comments. Happy to share the probe file if useful.
| shard->tenants.erase(tenant_it); | ||
| } | ||
| auto& tenant_state = *tenant_handle; | ||
| EraseMetadata(tenant_state, tenant_state.Get(durable_entry.object_key), |
There was a problem hiding this comment.
[P1] Durable metadata-erase finalizer still erases by key, not by entry identity.
This callback runs on the oplog writer's callback thread after durability. ClearStaleHandles queues it against a specific invalidated entry (master_service.cpp:2694), but here the key is resolved again at callback time and whatever entry is current gets erased. If the old entry is gone by then and a new object has been published under the same key (a retry Put after the stale-handle sweep is the natural flow), this silently deletes the replacement.
Reproduced deterministically: with a fresh valid entry under key K, invoking the finalizer with an old REMOVE entry for K erases it (ContainsObject(K) == false).
Fix: carry the entry identity (ObjectEntry::generation() captured at scheduling, or a pinned handle) and revalidate (IsCurrent / generation match) before erasing; only use the key-based fallback when the recorded entry is no longer the published one.
| dynamic_lease_id, dynamic_version_epoch, | ||
| ttl); | ||
| ? [this, ttl](const OpLogEntry& durable_entry) { | ||
| FinalizeExpiredProcessingReplicasAfterDurable( |
There was a problem hiding this comment.
[P1] Expired-processing finalizer pops whatever entry the key resolves to now.
This schedules FinalizeExpiredProcessingReplicasAfterDurable for the expired entry by key, but the callback re-resolves the key with a fresh accessor and pops all PROCESSING replicas of whatever entry it finds — it never checks that this is the same entry, nor even that its put_start_time matches the TTL that triggered the cleanup. If the old entry was preempted in the meantime (a PutStart after put_start_release_timeout, or an UpsertStart, both erase and re-create the key), the finalizer discards the new object's in-flight replicas and can erase it.
Reproduced deterministically: a fresh PROCESSING entry under the key plus the finalizer for an older entry -> CountReplicas() == 0 and the key is gone.
Fix: pass the entry identity/generation (or a pinned handle) into the callback and skip when the current entry is not the expired one. The REMOVE variant two branches down needs the same treatment.
| } | ||
| auto entry_lock = entry->LockShared(); | ||
| const ObjectMetadata& metadata = entry->metadata(); | ||
| if (!metadata.IsValid() || !HasReadableReplica(metadata)) { |
There was a problem hiding this comment.
[P1] Torn-down entries are still served by the manual read paths.
The accessors now check !entry->is_torn_down, but these paths open-code the check with metadata.IsValid() only, so they can serve an entry whose teardown already released quota/disk accounting and published the KV remove event — and BatchExistKey refreshes its lease a few lines below:
BatchExistKey(master_service.cpp:3125)GetReplicaListByRegex(:4060)GetReplicaListForAdmin(:4243,:4369)GetAllKeys(:3150) lists torn-down keys (minor)
Reproduced deterministically: with is_torn_down set on a published entry, ExistKey/GetReplicaList are correctly rejected, but BatchExistKey returns true and GetReplicaListByRegex still returns the key.
Fix: include !entry->is_torn_down in these manual checks (or route them through the accessors), so every read path agrees that a claimed teardown is not readable.
| // re-lock a mutex it already holds. | ||
| lock_ = std::unique_lock<std::shared_mutex>(); | ||
| entry_.reset(); | ||
| entry_ = tenant_state_->Get(object_id_.user_key); |
There was a problem hiding this comment.
[P1] Create() can adopt a torn-down entry, and AddReplica then acknowledges a registration for a key that is already gone.
Exists() returns false for a torn-down entry, so Create() builds and locks a new envelope. If the route still points at the torn-down entry at that moment, InsertObject fails and this fallback re-resolves the key and adopts whatever it finds — including an entry that has already been claimed for teardown but whose route slot has not been erased yet. AddReplicaForRetainedClient never re-checks Exists() after Create(), so it wires the disk replica into the torn-down entry and returns true; the teardown then erases the route.
Reproduced deterministically with a gate at the teardown's entry-unlock: AddReplica returns success while ContainsObject(key) == false.
Fix: if the fallback entry is torn down, retry the create (the route erase is imminent) or return a retryable error; and re-check accessor.Exists() after Create() before wiring.
| * Strict tenant admission and policy mutation paths that need both | ||
| * tenant_quota_policy_mutex_ and snapshot_mutex_ must acquire the tenant | ||
| * policy mutex first, then snapshot_mutex_. | ||
| * The per-object lock is never held together with the object route lock |
There was a problem hiding this comment.
[P2] The documented lock-order invariant is no longer true.
This (newly added) comment says the per-object lock and the route lock are never held together, but Create() (master_service.h:1678) and AllocateAndInsertMetadata (master_service.cpp:4643) now take entry->LockUnique() before InsertObject, which acquires the route lock — i.e. entry -> route nesting. The call-site comment at :1679 documents the weaker, accurate claim ("no path takes the route lock and then an entry lock"), and I could not find that order violated in the current tree, but the general invariant here and the same wording in EvictGroupOrObject (:1200) should be updated — otherwise future route -> entry code (or the HA route-lock gating tests) will rely on a rule the tree no longer follows.
| // There is no per-tenant mutex; serialize RemoveAll with an exclusive | ||
| // snapshot lock so concurrent RemoveAll calls do not double-count/tear | ||
| // down. | ||
| std::unique_lock<std::shared_mutex> shared_lock(snapshot_mutex_); |
There was a problem hiding this comment.
[P2] Tenant-scoped RemoveAll now blocks metadata requests across every tenant.
This changes the snapshot barrier from shared to exclusive and holds it for the entire tenant scan and teardown loop. Normal GetReplicaList, ExistKey, PutStart, and PutEnd requests also acquire snapshot_mutex_, so clearing a large tenant A prevents even requests for unrelated tenant B from proceeding until A's cleanup finishes. The baseline held the snapshot barrier shared and serialized mutations through the individual shard locks; this introduces a global pause rather than merely changing the object-lock granularity.
Avoiding duplicate teardown/counting is necessary, but could we keep the snapshot barrier shared and enforce that through the per-entry teardown claim and its return value, or separate appropriately scoped serialization? The global RemoveAll(bool) overload has the same new exclusive-lock pattern and should be considered alongside this one.
A regression test could pause tenant A's cleanup after it enters the scan and verify that a point read/write against tenant B can complete before A is resumed. This would explicitly protect the cross-tenant isolation that the object-level locking change is intended to improve.
- Serve torn-down entries nowhere: gate BatchExistKey, GetAllKeys, GetReplicaListByRegex and both batch replica-list paths on the teardown flag (GetReplicaListForAdmin is already covered by the MetadataAccessorRO::Exists gate). - PutStart and MetadataAccessorRW::Create no longer adopt a torn-down entry; AddReplicaForRetainedClient re-checks the accessor so a registration cannot be acknowledged for a key that is logically gone. - Address deferred finalizers by pinned entry: both re-resolved the key at durable time and could act on a same-key recreate; FinalizeExpiredProcessingReplicasAfterDurable additionally gates on the current index slot. - Hold the RemoveAll snapshot barrier shared and count erases only on a won teardown claim (EraseMetadataLocked now returns bool), so a tenant wipe no longer blocks metadata traffic for unrelated tenants. - Document the real lock order: entry -> route is allowed, route -> entry is forbidden.
…ndex Route lookups hash a string_view directly instead of materializing a temporary std::string per request: the main table now uses a transparent hash (StringHash, is_transparent via std::hash<std::string_view>) with std::equal_to<>, whose equivalence semantics match the previous std::equal_to<std::string> exactly. The read-only lookups (Get, IsCurrent, Contains, WithObject) widen to std::string_view, and TenantCatalog::Get forwards the widened form — otherwise callers holding a view would still pay the construction at the catalog boundary. Insert/EraseIf keep their signatures. TenantCatalog's constructor reserves 4096 route buckets as a pre-sizing hint — rehash behavior stays the standard-library default with no cap. Measured (metadata_scan_bench, 100k keys, median of 3): a lookup from a string_view drops from 687ns to 482ns (-30%, one temporary 17-byte string construction eliminated); a lookup from an existing std::string lvalue is unchanged (430ns -> 437ns, within noise). Tests: object_index_test, tenant_catalog_test, and the full master_service/scenario/HA/tenant/promotion suite all pass on the remote build (mooncake_master target builds clean).
…object index" This reverts commit 78df7e5.
Pause a tenant-scoped RemoveAll on one entry's lock and assert that a point write against another tenant still completes. With the old exclusive snapshot barrier this deadlocks until the watchdog kills it; with the shared barrier it passes.
Ports the leased-upsert split (AllocateReplicas + InsertMetadata) to the tenant-first metadata access path: allocation is shared verbatim, and InsertMetadata operates on the tenant container instead of the removed shard accessor.
SnapshotAllocatorManager deep-copied the segment->allocator registry (names, registration vectors, owner map) on every PutStart, put commit, and replicate-target allocation. Cache the snapshot copy-on-write: rebuild only after a registration or owner-map mutation invalidates it, and hand every reader the same immutable instance.
stmatengss
left a comment
stmatengss
left a comment
There was a problem hiding this comment.
There are a lot of code changes. Please split the extensive code changes into multiple smaller PRs.
|
Benchmark numbers for the current head vs the pre-tenant-first baseline (ca09c18), measured with Workload: 8 segments x 400 GiB, 448 KiB values (the sglang-derived defaults),
Each cell is round 1 / round 2. What the numbers show:
Update (after the upstream/main merge): Profiling on an idle machine showed the head executes 1.54x the instructions of the baseline with no elevated cache-miss or stall rates, so the gap is straight-line work, not memory stalls. The largest single contributor was |
rishabhsinha17
left a comment
rishabhsinha17
left a comment
There was a problem hiding this comment.
Reviewed the test and DSL lane on 1c1835d, wearing the MasterScenario DSL author hat (#3847, #3927). I read the full diff against merge base 6ae74b3 and ran the affected suites locally at the head, numbers at the end. Aionw's locking findings stand on their own and I did not re-verify them.
The DSL side is in good shape. This PR leaves scenario.h and scenario_contract_test.cpp untouched, so the grammar and its pins hold. Rewriting the ExpireAt engine preserved the verb's contract. Both miss paths keep the old failure text, the rewrite mutates the same two fields, and the new ForTesting setters take the same spinlock the old inline code took. One catalog_.Lookup plus the entry lock keeps the white box surface as narrow as before, and follows the locking rules this PR introduces. Our #3927 scenario suites run unmodified. Flat group all or none eviction matches what EvictsWholeGroupTogether and ActiveGroupMemberBlocksWholeGroup pin, so the model and the scenarios agree.
Most of the test deletions are sound and worth putting on record. ShrinkBucketsIfSparse and its two tests go away with the helper, and the PR body prices the deferred compaction. ReRouteRestoredObjectsMovesStaleShardObjects tested a migration that cannot exist without placement. QueueLimitRejectsCrossShard collapses into QueueLimitRejectsBeyondCap once shards are gone. GroupedRoutingUsesHashOfTenantAndKeyOnly pinned shard hash math, and group_index_test now covers its membership half. And the renamed ClearInvalidHandlesSweepPreservesCoverage keeps the erase all, survive all assertions.
Not every deletion is free, though. Dropping both commit versus clear ordering race tests left the suppression branch they pinned uncovered while it is still live, and the fixture gained AccessorCreateRePinsWinnerEntry with no TEST_F invoking it. Both are cheap to fix, details inline.
On the additions, ConcurrentFirstWritesToDistinctKeysShareReachableTenant pins get or create atomicity, CandidateIndexMirrorsEntryState pins the sparse candidate index against drift, and TenantRemoveAllDoesNotBlockOtherTenants proves the shared barrier claim with a held entry lock gate. object_index_test pins EraseIf identity and generation ordering, the index level form of the removal invariants the concurrent scenarios exercise. Good coverage to have.
Local runs at head 1c1835d, same Ubuntu 22.04 container as my #3927 runs, RelWithDebInfo, gcc 11, USE_ETCD=OFF. The thirteen DSL suites come back green, master_scenario_test 44/44, evict 22/22, concurrent 10/10, offload 25/25, ssd 9/9, base scenario 32/32, group 13/13, placement 12/12, replication 16/16, segment 15/15, task 9/9, tenant 10/10, dfs 6/6. Suites this PR edits pass as well, master_service_test 59/59, group_test 11/11, double_erase 1/1, tenant_quota_test 31/31, promotion_on_hit_test 55/55, and the five new unit suites come out 20 for 20. I did not run the HA or client integration matrix.
A rebase is due again. Current main conflicts in master_service.h, master_service.cpp and master_service_tenant_quota_test.cpp, from #4052 and #3805. Worth folding the #3805 per tenant eviction watermark into TenantCatalog during that rebase, it fits this model naturally.
Two DSL follow-ups are mine to take once this merges, renaming GroupOnDifferentShard since the name encodes the removed sharding model, and a contract test pinning ExpireAt's failure report. Please keep both out of this PR given the split request already standing.
| // `cleared` would order it after that commit's `stored` — telling subscribers | ||
| // to drop an object that is live. Pause the scan right after the shard the new | ||
| // key belongs to, commit there, and the clear must be withheld. | ||
| TEST_F(MasterServiceTest, ConcurrentCommitDuringScanSuppressesClear) { |
There was a problem hiding this comment.
Deleting this and TenantScopedRemoveAllSuppressesClearOnRace removes the only coverage of the suppression branch in PublishKvClearedIfEpochUnchanged, and that branch is still live. Both RemoveAll overloads read their epoch baseline before the walk and decide per tenant afterwards. The race stays as reachable as it was under shards. Only the uncontended mirror test remains, and it pins the publish side, which leaves kv_cleared_suppressed_by_epoch_ free to regress silently. Your shard hook is gone, but re-expressing this does not need a new hook on MasterService. Your own gate technique from TenantRemoveAllDoesNotBlockOtherTenants reproduces the race deterministically. Park a forced RemoveAll on a held entry lock, then commit a fresh key into the same tenant while the sweep waits on the gate. That key misses the walk's SnapshotObjects snapshot and its PutEnd bumps the epoch past the baseline. Release the gate, join, and expect suppressed 1, published 0, and the racer still readable. One more copy of that gate covers the tenant scoped overload.
| // would bind to an orphan entry not in the route. It must re-resolve the | ||
| // existing entry instead. Lives on the fixture (a MasterService friend) so | ||
| // it can reach the private MetadataAccessorRW. | ||
| void AccessorCreateRePinsWinnerEntry(MasterService& service) { |
There was a problem hiding this comment.
No TEST_F invokes this, so the orphan rebind regression it documents goes unpinned. A one line TEST_F in master_service_test.cpp calling it closes that.
| if (update(routed)) { | ||
| // All of a tenant's keys live in one tenant container, so a single | ||
| // directory lookup finds the object (there is no per-shard probe). | ||
| auto tenant_handle = service_->catalog_.Lookup(tenant); |
There was a problem hiding this comment.
Faithful rewrite from where I sit as the DSL owner. Both miss paths keep the old failure text, the same two fields change, and the setters take the spinlock the old inline code took. Dropping the all shards probe is right, it existed only for placements the flat index cannot produce. I will take two follow-ups on my side once this merges, renaming GroupOnDifferentShard and a contract test pinning ExpireAt's failure report. Nothing for you to do here.
| #include <algorithm> | ||
| #include <atomic> | ||
| #include <chrono> | ||
| #include <atomic> |
There was a problem hiding this comment.
Duplicate, <atomic> is already included two lines up.
| // the TenantCatalog non-empty. All of a tenant's keys live in a single | ||
| // TenantCatalog regardless of shard (there is no per-key shard to share), | ||
| // so a simple suffix suffices instead of a shard-index probe. | ||
| std::string FindKeyOnSameShard(MasterService& service, |
There was a problem hiding this comment.
The helper name and the SAME shard wording here and in the file header describe the old model, while the body is now a plain suffix. Worth renaming to something like MakeKeepaliveKey while you are in this file.
Resolve the conflicts introduced by three upstream Store changes: - kvcache-ai#3805 (per-tenant eviction watermark): the write path no longer retries inline eviction, so ChargeTenantQuota and AllocateAndInsertMetadata drop the quota-deficit out-parameter and PutStart/UpsertStart call admit() once. EvictTenantsOverWatermark and its eviction-thread tick land on the tenant-first catalog unchanged. - kvcache-ai#4103 (shrink metadata maps after sweeps): the shard-level kStaleHandleCleanupBatchSize/shard loop does not apply, but the leak it fixes does. ShrinkBucketsIfSparse moves to common/shrink_buckets.h and is applied to each tenant's object route after ClearStaleHandles sweeps and after BatchEvict, using the evicted_tenants list that was collected but never consumed. - kvcache-ai#4052 (oplog writer metrics): only the metric call in master_service.cpp. The tenant-first model keeps its deliberate divergence: empty tenants stay in the directory (an Empty()+Remove() race would strand a just-inserted object in an unreachable catalog), so the upstream per-sweep tenant erase is not carried over.
…eams ClearStaleHandles and BatchEvict now shrink the per-tenant object route, so the two upstream shrink tests are re-expressed against ObjectIndex instead of the removed metadata shards (no shard selection: a tenant's keys all live in one route). Restore the coverage the shard-era tests took with them: the suppression branch of PublishKvClearedIfEpochUnchanged had no test that forces it. Pin it twice -- deterministically through the fixture seams, and end to end by parking a forced RemoveAll on a held entry lock and committing into the same tenant inside the window. Also: give AccessorCreateRePinsWinnerEntry the TEST_F it never had, rename FindKeyOnSameShard to MakeKeepaliveKey now that shards are gone, drop a duplicate <atomic> include, and remove two comment blocks left over from the deleted ReRouteRestoredObjects and the removed group-membership forwarders.
The tenant factory runs synchronously under the directory's write lock today, so capturing the tenant id by reference is safe but silently depends on that. Capture by value and document why the factory stays a call-site argument rather than a catalog member.
thanks your review, and I will split this large PR into multiple smaller ones and take your suggestions into account. |
Description
Implements object-level metadata locking for the Mooncake Store (issue #3670, flat-group model from #3589).
Today a single coarse shard lock serializes all writes and eviction on a shard, so unrelated objects contend on one lock. This PR replaces it with a per-object lock (
ObjectEntry::mutex) plus a per-tenant object route: an object is excluded only while it is itself being read, written, or evicted. Group semantics keep the routing decoupling of #3589 in a thinner form — a group is a membership table plus one sharedLease(GroupIndex), consulted only by eviction for all-or-none decisions; the read path never touches the group table.New modules under
mooncake-store/include/tenant/:MetadataCatalog— boundary between MasterService and the tenant module; owns the tenant registry and lifecycle rules.TenantDirectory— copy-on-writeTenantId→TenantCatalogmap; lock-free reads, atomic get-or-create.TenantCatalog— per-tenant aggregate: quota handle,ObjectIndex,GroupIndex.ObjectIndex— flat key →shared_ptr<ObjectEntry>route with identity-checked erase (EraseIf), publication generations for remove/recreate races, and the dynamic-replication lease table.ObjectEntry— per-object mutex guarding the metadata envelope and all per-key task state that previously lived in parallel shard maps.ObjectMetadata/object_replica_list.h— extracted metadata envelope (identity, replicas, lease, soft-pin, quota ledger).Concurrency rules: route locks are never held together with object locks (strong handles bridge the gap), teardown is claimed once per entry (
is_torn_down), and a route slot is erased only against the exact entry being torn down, so a concurrent remove + re-create cannot drop a live object.Module
mooncake-transfer-engine)mooncake-store)mooncake-reshard)mooncake-ep)mooncake-pg)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-common)mooncake-rl)Type of Change
How Has This Been Tested?
Unit / regression suites (all pass):
master_service_test(69),master_service_ha_test(93),tenant_store_test(15),tenant_directory_test(4),object_entry_test(1), plus newgroup_index_test,object_index_test,tenant_catalog_test,tenant_directory_test.Performance A/B (Debug builds, same machine and compiler, same workload). baseline = pre-PR master (
ca09c18d, shard locks), this PR =7960b555. 2x4GiB segments, 1MiB values, fixed arrival, groups of 5 consecutive put keys share one group_id, eviction driven to the 0.8 high-watermark on both.Metadata plane (client→master RPC,
master_synthetic_bench), 60s per run:p99 -84.9% / -85.9%, p999 -89.6% / -89.8%; p50 -0.8% / -13.7%.
rpc_failure_eventsalign on both sides (148 vs 148, 390 vs 390) and item_qps are identical, so the comparison is same-load. The shard-lock baseline tail is also unstable across runs (medium p99 varied 58-79 ms between rounds); this PR stays in a 9-13 ms band.Isolation under churn. One client writes 1MiB objects at full speed to keep eviction continuously active; a second client runs a mixed read/write workload against the same master. Victim p99: baseline 52.9 → 85.9 ms under churn; this PR 28.6 → 46.4 ms. Under churn, this PR's victim tail stays below even the baseline's uncontended p99 (46.4 vs 52.9 ms).
2x load, 16 workers (metadata plane, fixed arrival at ~8695 item/s on both): baseline p99 105.5 ms, this PR 12.4 ms;
rpc_failure_eventsidentical (876 both). This PR's p99 stays in the 9-13 ms band from 1x to 2x load.Memory cost. Master RSS measured over a 90s linear small-object write (~1M objects): baseline ~0.8 KB/object, this PR ~1.2 KB/object (+~0.4 KB, about +50%). The increase is the per-object lock, generation and task state folded into
ObjectEntry, plus the route key stored next toObjectMetadata::user_key— roughly 4 GB of master RSS at 10M objects. This is the cost of the per-object locking model; compaction (single key storage, on-demand task allocation) is left as follow-up work.End-to-end runs (
store_kv_bench, real data movement through the transfer engine, eviction continuously active) show the same direction: throughput 1800 → 2788 MiB/s and p99 52.9 → 28.6 ms. Note the store bench is closed-loop, so those two runs are not same-load; the metadata-plane tables above are the strict comparison. Raw logs available on request.The branch is rebased onto current
upstream/main; tenant module headers live underinclude/tenant/.Checklist
./scripts/code_format.shAI Assistance Disclosure