[Store] Add the per-object entry, object route and group table - #4161
Conversation
The three storage primitives the tenant-first metadata model is built from: - ObjectEntry owns the ObjectMetadata envelope, adds the per-key task state, and carries the per-object mutation boundary and the route generation. - ObjectIndex is one tenant's object route, a flat map from object key to a strong ObjectEntry handle, plus the in-flight dynamic-replication lease table. It routes by the entry's own key, so the slot an entry lands in and the key every lookup and erase uses cannot disagree, and it hands out strong handles that mean the entry is alive rather than still current: a caller revalidates with IsCurrent or erases with the identity-checked EraseIf. - GroupIndex is group id to one shared Lease plus its member keys. A group is not a container of objects: the lease is the all-or-none unit eviction reads, and the read path extends it on a member hit without touching this index. Nothing constructs these types yet, so this changes no behavior; the service that owns the equivalent shard tables is converted separately.
ObjectMetadata sits in mooncake:: (it was extracted there), so ObjectEntry and ObjectIndex now sit beside it rather than one level deeper. GroupIndex stays in mooncake::metadata with the tenant aggregate that will own it.
ObjectIndex carried two responsibilities: the object route and the table of in-flight dynamic-replication leases. The lease table is keyed by proposal, not by object, so a client's in-flight action is not an index concern; it is now DynamicReplicationLeaseTable, and ObjectIndex.Empty only answers for its own route. Also drops the two members no caller reached: WithObject (Get plus a locked callback, with no production call site) and IsCurrent (the revalidation the dynamic-replication path performs itself by comparing generations), and takes the entry handle in EraseIf so a caller cannot pass an entry it no longer holds.
The group table holds one stripe per hash bucket, each stripe is a map plus a lock (112 B), and every tenant holds one table, so the count buys write concurrency with tenant memory. It was a constant chosen from a single before/after measurement; StripedGroupIndex makes it a compile-time parameter and the default now records the curve it was picked from. Measured on a 256-core box (member adds/s at 32 threads, one thread per group prefix, 4096 groups per thread): 6.97M at 32 stripes, 12.53M at 64, 15.40M at 128, 18.79M at 256, against 7 KiB per tenant at 64 and 15 KiB at 128. 64 is the last large step; the curve has not flattened by 256, so a grouping-heavy workload can raise the count. The test envelope also takes a fixed write time instead of reading the clock.
…dex at the root Review feedback on the object model: - ObjectEntry no longer hands out its lock or its state. The per-key task state is a nested State reachable only through WithExclusiveAccess or WithSharedAccess, which hold the lock for the callback and return what the callback returns, so a caller cannot act on half of a compound operation. That removes the three lock accessors, the exposed state fields and the unlocked metadata() reference. Exclusive access needs a mutable entry, so only the shared accessor is const and the mutex is mutable. - group_index.h moves out of include/tenant/. Grouping is a property of the objects, not of tenancy, so it sits with the rest of the object model and in the same namespace. - Comments that only restated the statement they sat above are dropped, and the lease table's Empty() assertions move into the test for the table it answers for instead of a test of their own.
There was a problem hiding this comment.
🟡 Changes recommended
Critical correctness and concurrency findings remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds tenant-first Store primitives for object metadata, routing, group leases, and dynamic replication leases, with accompanying tests.
Changes:
- Adds
ObjectEntryandObjectIndex. - Adds
GroupIndexandDynamicReplicationLeaseTable. - Adds shared test helpers and registers four test suites.
File summaries
| File | Summary | Review findings |
|---|---|---|
mooncake-store/tests/object_test_helpers.h |
Shared object test builders | No final findings. |
mooncake-store/tests/object_index_test.cpp |
Object routing tests | No final findings. |
mooncake-store/tests/object_entry_test.cpp |
Object entry tests | No final findings. |
mooncake-store/tests/group_index_test.cpp |
Group membership and lease tests | No final findings. |
mooncake-store/tests/dynamic_replication_lease_table_test.cpp |
Dynamic lease table tests | No final findings. |
mooncake-store/tests/CMakeLists.txt |
Registers new test suites | No final findings. |
mooncake-store/include/object_index.h |
Key-to-entry object route | Critical (1 vote): synchronize generation_ publication with concurrent reads. |
mooncake-store/include/object_entry.h |
Per-object metadata and runtime state | Critical (1 vote): synchronize generation_ reads and writes. |
mooncake-store/include/group_index.h |
Group membership and shared leases | Critical (3 votes): avoid materializing a lease for an empty group ID. Moderate (1 vote): handle lease allocation failure without retaining broken state. |
mooncake-store/include/dynamic_replication_lease_table.h |
Proposal-keyed replica leases | Critical (3 votes): include <mutex> directly. Moderate (1 vote): validate or derive the map key from lease.proposal_id. |
Review details
Suppressed comments (3)
mooncake-store/include/dynamic_replication_lease_table.h:41
Putaccepts the map key separately fromlease.proposal_idand never verifies that they match. A mismatched call leavesFind(proposal_id)returning a lease whose embedded proposal identity names another action, so proposal-keyed consumers can operate on inconsistent state. Make the lease ID authoritative or reject mismatches before storing.
void Put(const UUID& proposal_id, ReplicaActionLease lease) {
std::unique_lock<std::shared_mutex> lock(mutex_);
leases_[proposal_id] = std::move(lease);
mooncake-store/include/group_index.h:45
- If
std::make_shared<Lease>()throws aftertry_emplacehas inserted the emptyGroupState, that broken state remains in the table. A later call skips lease initialization, adds a member, and returns a null lease. Construct the lease before insertion or roll back the newly inserted group on allocation failure.
auto [it, inserted] = stripe.groups.try_emplace(std::string(group_id));
if (inserted) {
it->second.lease = std::make_shared<Lease>();
}
mooncake-store/include/object_entry.h:69
decltype(auto)forwards reference returns fromfn, so a callback can returnObjectMetadata&orState&and let that reference escape afterlockis destroyed. That breaks the callback-scoped locking contract and permits unsynchronized access to the entry; return a value (or reject reference result types) instead.
decltype(auto) WithExclusiveAccess(Fn&& fn) {
- Files reviewed: 10/10 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| #include <chrono> | ||
| #include <cstdint> | ||
| #include <optional> | ||
| #include <shared_mutex> |
| // a member). One step, so the lease and the membership cannot disagree. | ||
| [[nodiscard]] std::shared_ptr<Lease> AddMember( | ||
| std::string_view group_id, std::string_view member_key) { | ||
| auto& stripe = StripeFor(group_id); |
| // Monotonic generation assigned by ObjectIndex at route publication | ||
| // (0 = never published). Lets a holder distinguish the entry it holds from | ||
| // a later replacement of the same key. | ||
| [[nodiscard]] uint64_t generation() const noexcept { return generation_; } |
| if (!inserted) { | ||
| return false; | ||
| } | ||
| it->second->generation_ = ++generation_counter_; |
AddMember keeps the contract of MasterService::RegisterGroupMember, which this index is extracted from: it returns the group's single shared Lease, re-registering a member returns that same lease, and the one null means an empty group_id, where the object is not grouped and there is no lease to wire. The table no longer materializes a group for an empty id, which would have put every ungrouped object on one lease and expired them together, and a repeat is no longer reported as a null lease. Empty() answers from a group count kept under the stripe locks rather than taking one lock per stripe.
Publication writes generation under the route lock while a holder reads it through generation() with no lock at all, which is a data race on the plain uint64_t. The field is now atomic, relaxed: the number is only compared, it carries no other state.
The table kept one map and answered two of its three questions by scanning it: a teardown retracted the leases for a key by walking the whole table, and the expiry sweep walked it again. Three indexes now share the one lock. The proposal id still finds the lease, the object key finds the proposals in flight for it, and a deadline heap orders the sweep, so neither query scans. A node is validated against the lease it names when it surfaces, which is how a removed or extended lease leaves its superseded deadline behind harmlessly. The header also includes <mutex> for std::unique_lock instead of relying on a transitive include.
SharedLeaseWiresGroupAllOrNoneExpiry restated what AddMemberCreatesAndSharesOneLeasePerGroup already asserts, that distinct groups hold distinct leases, and then exercised Lease's own grant and expiry API, which is not this unit: master_service_group_test covers lease expiry, and lease.h brings its own semantics. UngroupedMemberHasNoLease drops an assertion that Empty() already makes.
HasLeaseForObjectForTest existed for the suite alone: nothing reaches a lease by object key, production reaches it by proposal. The suite now asserts the same invariant through EraseForObject and Find, which is the behaviour the accessor stood for, so the table exposes only what it is asked for.
| if (segment_name.empty()) | ||
| return Status::InvalidArgument("Empty segment name" LOC_MARK); | ||
| CHECK_STATUS(ControlClient::getSegmentDesc(segment_name, response)); | ||
| // A pooled RPC connection to a peer that has gone away is only discovered |
There was a problem hiding this comment.
Since this is actually a tent-side change, please open an issue and submit this commit as a tent PR. And this fix here need further investigation.
2c9f2cb to
ec187b2
Compare
|
CI failure addressed in #4210 |
Description
Adds the storage primitives the tenant-first metadata model is built from:
mooncake-store/include/object_entry.hObjectEntry: owns theObjectMetadataenvelope, adds the per-key task state, and keeps its lock private behindWithExclusiveAccess/WithSharedAccessmooncake-store/include/object_index.hObjectIndex: one tenant's object route, a flat map from object key to a strongObjectEntryhandlemooncake-store/include/group_index.hGroupIndex: group id → one sharedLeaseplus the member keys that point at itmooncake-store/include/dynamic_replication_lease_table.hDynamicReplicationLeaseTable: the in-flight replica-action leases for one tenant, keyed by proposal idAll four sit in
mooncake::besideObjectMetadata. Group membership is a property of the objects, not of tenancy, so the group table does not live underinclude/tenant/.RFC #3670 moves metadata ownership from global shards to tenants and narrows the mutation boundary from one shard lock to one object lock. These are what that model stores: the per-object shell, the route that finds it, the group table that makes a group evict all-or-none, and the leases for the replica actions a client has in flight. Today the same state sits in parallel per-key maps on the shard, where one lock covers unrelated objects and a compound operation touches several maps under it.
No behavior change: nothing constructs these types yet.
What the types promise
ObjectIndex::Getreturns the entry under the shared route lock and releases that lock before the caller takes the entry's own lock, so a caller revalidates through the identity-checkedEraseIfor the entry'sgeneration()instead of trusting its handle.generation()is atomic, because publication assigns it under the route lock while a holder reads it under no lock.Insertroutes byentry->key(), so the slot an entry lands in and the key every lookup, revalidation and erase uses cannot disagree.TransparentStringHashand takestd::string_viewon the lookup paths, so a caller that already holds a view does not build a string to find an entry.WithExclusiveAccess/WithSharedAccess, which hold the lock for the callback and hand out the envelope and the state together; the lock order is entry lock → route lock → metadata spin lock, never the reverse for any pair.Lease.AddMembermaterializes the group, registers the member and returns that lease under one lock section, so a lease and its membership cannot disagree; the read path extends the lease on a member hit without touching this index, and eviction reads it to decide the whole group at once. Re-registering a member returns the same lease, and an empty group id is the ungrouped case: it registers nothing and returns no lease.Module
mooncake-store)Type of Change
How Has This Been Tested?
Build and the store regression set on a 256-core Ubuntu box, Debug + Ninja,
USE_ETCD=ON, GNU 11.Test commands:
Test results:
object_entry_test2,object_index_test5,dynamic_replication_lease_table_test6,group_index_test5, 18 in total — covering the route (publish, identity-checked erase, generations, duplicate rejection, snapshot enumeration), the entry (the envelope it owns, the state a shared reader sees), the lease table (proposal-keyed lookup, replacement, retraction by object key, the expiry sweep against removed and extended leases) and the group table (one shared lease per group, membership add/remove/repeat, the ungrouped case, the group dropped with its last member).clang-format --dry-runreports no violations on the files this PR adds.Checklist
./scripts/code_format.shAI Assistance Disclosure