Refactor: umbp metadata store rebase#437
Open
TianDi101 wants to merge 15 commits into
Open
Conversation
Preparatory type changes ahead of the IMasterMetadataStore refactor. No
behavior change; the rest of the refactor becomes purely structural.
0a. Hoist NodeTierKey (from GlobalBlockIndex) and NodeMatch (from
ExternalKvBlockIndex, with MatchedHashCount) to types.h. Temporary
`using` aliases left in both classes so existing callers compile;
removed in Phase 5.
0b. Add ClientRegistration and HeartbeatResult (with nested
enum Status { APPLIED, SEQ_GAP, UNKNOWN }) to types.h. Additive;
these are the input/result vocabulary the Phase 1 interface uses.
0c. Migrate boundary-crossing timestamps from steady_clock to
system_clock (hazard #7): ClientRecord {last_heartbeat,registered_at},
BlockMetrics {created_at,last_accessed_at}, EvictionCandidate
last_accessed_at, BlockEntry lease/access atomics + GrantLease/
BatchLookupForRouteGet durations, Router lease_duration_, and
master_server NowNs(). Process-local duration/timeout clocks (rpc
latency timer, ssd read-lease, peer allocator deadlines) intentionally
stay steady_clock. Fixes the one audit fallout in
test_route_put_strategy.cpp that constructed the migrated fields.
Full UMBP C++ unit suite (local + distributed) green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Land the abstract IMasterMetadataStore interface header consolidating the four master-side state holders (GlobalBlockIndex, ClientRegistry, ExternalKvBlockIndex, ExternalKvHitIndex) behind one contract. No consumers wired yet. - master_metadata_store.h: lifted from the draft, depends only on types.h. Adds the two hit-count methods the draft dropped (GetExternalKvHitCounts, GarbageCollectHits) so the live GetExternalKvHitCounts RPC path is preserved, and adds a `now` parameter to MatchExternalKv so count_as_hit=true can stamp last_seen. - Hoist EvictionCandidate from global_block_index.h into types.h (part of the store contract; mirrors the phase 0 NodeTierKey/NodeMatch hoist) so the interface depends only on types.h. - Compile/instantiation gates: self-compile TU, GMock MockMasterMetadataStore (reused in phase 3), and a signature-completeness test exercising every method through IMasterMetadataStore&. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Implement IMasterMetadataStore in-process by folding the four former state holders (GlobalBlockIndex, ClientRegistry, ExternalKvBlockIndex, ExternalKvHitIndex) behind a single std::shared_mutex. - Block locations + LRU/lease (per-entry atomics mutated under a shared lock, keeping the RouteGet hot path concurrent); lease/access timestamps are now caller-supplied system_clock (hazard #7). - ApplyHeartbeat single-seq CAS; SEQ_GAP keeps liveness but not caps/seq (hazard #1). ExpireStaleClients flips ALIVE->EXPIRED and keeps the row, cascading block + external-KV cleanup atomically (hazard #3). - RegisterExternalKvIfAlive fuses the alive-check with the write (TOCTOU fix). - MatchExternalKv(count_as_hit=true) is the one formerly-shared path that takes the unique lock; hit last_seen is system_clock and feeds GarbageCollectHits. - EnumerateLruForEviction = Option A (full scan + sort + greedy byte budget, no maintained index) so tie-timestamps never drop candidates. Adds the §6a behavioral suite (31 cases) written against IMasterMetadataStore& so it is reused for the Redis backend; all green, concurrency cases clean under ThreadSanitizer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the direct GlobalBlockIndex / ClientRegistry / ExternalKvBlockIndex / ExternalKvHitIndex references in every master-side consumer with a single IMasterMetadataStore&. The four old classes are still present, so this isolates rewire regressions from the Phase 4 deletion. - Router: holds one store_ instead of index_ + registry_. RoutePut / BatchRoutePut read ListAliveClients() and BatchExistsBlock(); BatchRouteGet now passes an explicit system_clock::now() into BatchLookupBlockForRouteGet (the old GlobalBlockIndex read the clock internally — the timestamp now crosses the store boundary, hazard #7). - EvictionManager: constructor takes the store. RunOnce passes its existing per-(node,tier) byte budget (already computed down to the low watermark) straight into EnumerateLruForEviction and drops its own std::sort + greedy budget walk, since the store returns candidates LRU-ordered and budget-trimmed. The standalone overloaded-set is gone — the budget map's keys already identify the overloaded buckets. - MasterServer: the four state members collapse to one unique_ptr<IMasterMetadataStore> (InMemoryMasterMetadataStore), declared before router_/service_ so it outlives their references. gRPC handlers rewired to store_ calls. The Heartbeat handler flattens EventBundle[] into per-bundle single-seq ApplyHeartbeat calls and short-circuits on SEQ_GAP (§3e). The client-expiry reaper moves out of ClientRegistry into MasterServer (per-tick store_->ExpireStaleClients(cutoff)); the hit-index GC tick becomes store_->GarbageCollectHits(now - max_age) — both cutoffs on the system_clock basis so they compare against last_heartbeat / last_seen. - Add UnregisterExternalKvByNode to the interface, in-memory impl, and mock: it backs the live RevokeAllExternalKvBlocksForNode RPC (whole-node external-KV wipe that leaves the client record + block locations intact), which the §1b draft had dropped. Tests: - test_router_dedup migrated to construct InMemoryMasterMetadataStore instead of GlobalBlockIndex + ClientRegistry. - Store suite extended with the previously-uncovered methods: UnregisterExternalKvByNode (distinct from UnregisterClient), GetPeerAddress (ALIVE + EXPIRED + unknown), GetClientTags, and ListAliveClients content; the interface "every method callable" gate now names UnregisterExternalKvByNode. - InMemoryMasterMetadataStore behavioral suite green (36 cases) via the standalone g++ build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
UMBP C++ tests were split across two trees with two CMake wirings and a duplicate GoogleTest FetchContent: the standard tests/cpp/umbp and a second suite under src/umbp/tests (built unconditionally, with its own gtest fetch). Move the src/umbp/tests suite into tests/cpp/umbp/distributed/ (master/peer/ index/store/router logic = distributed layer), merge its target definitions into distributed/CMakeLists.txt (keeping gtest_discover_tests registration), and drop the src/umbp/tests subdirectory and its duplicate googletest fetch. The whole UMBP suite now builds under the single BUILD_TESTS + BUILD_UMBP gate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Now that all consumers route through IMasterMetadataStore (Phase 3),
remove the four superseded master-side state holders and their
class-specific test suites:
- GlobalBlockIndex, ClientRegistry, ExternalKvBlockIndex,
ExternalKvHitIndex (header + cpp each)
- their Phase 0a `using NodeTierKey` / `using NodeMatch` aliases
(lived inside the deleted headers)
- GlobalBlockIndex::GetMetrics (zero callers outside the class)
Behavioral coverage moved to the store suite
(test_in_memory_master_metadata_store), so the five class-specific
tests are dropped. Two suites that still constructed the old classes
as fixtures are migrated to InMemoryMasterMetadataStore instead:
- test_ssd_reliability: ApplyEvents/Lookup/BatchLookupForRouteGet ->
RegisterClient + ApplyHeartbeat (ascending seq) + LookupBlock /
BatchLookupBlockForRouteGet
- test_umbp_tags Suite 1: ClientRegistry -> store
RegisterClient/ApplyHeartbeat/GetClientTags/UnregisterClient
BlockMetrics and EvictionCandidate stay in types.h (still used by the
store's BlockEntry, eviction_manager, and test_types).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n docker The UMBP C++ tests were consolidated under tests/cpp/umbp/distributed (commit 00652fa); remove the now-duplicate originals left behind under src/umbp/tests (their CMakeLists was already deleted in that commit, so these files were orphaned and unbuilt). Also add .rocprofv3/ to .dockerignore so profiler output stays out of the docker build context. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Salvaged from the dropped "add profiling logs" commit (7892877c), which was built on main's pre-#382/#403 routing API and was auto-dropped as redundant during the rebase onto main. Only the parts not already on main are kept here; the RoutePut half (per-key Select vs BatchSelect) is gone because main replaced TierAwareMostAvailableStrategy::Select with ConfigurableRoutePutStrategy::SelectBatch, leaving nothing to compare. - RouteGetStrategy gains BatchSelect(): one virtual dispatch for a whole BatchRouteGet instead of one Select() per key. Default impl loops over Select() so custom strategies that only override Select() keep working; Random/TierPriority override it with a tight internal loop. Empty candidate lists are left as a default Location ("not routed") and skip Select(), mirroring the router's pre-batch skip. - Hoist the per-strategy selection cores into PickRandomReplica / PickTierPriorityReplica shared by Select and BatchSelect. The per-call MORI_UMBP_DEBUG lines that ran SummarizeLocations() unconditionally (macro args evaluated even when the level suppresses output) are commented out — they dominated the hot path. - Router::BatchRouteGet calls get_strategy_->BatchSelect once over the whole batch. - Add BatchSelect unit tests and a RouteGet-only micro-bench (bench_route_strategy.cpp, not a CTest target). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Three RouterDedup cases that main carried in src/umbp/tests/test_router_dedup.cpp were dropped when that file was deleted in the test consolidation (they used the old GlobalBlockIndex/ClientRegistry API). Re-add them against InMemoryMasterMetadataStore so the coverage survives the refactor: - RoutePutMarksAlreadyExistsForIndexedKey: single-key RoutePut delegates to the batch path, so dedup still applies (indexed key -> kAlreadyExists, unknown -> kRouted). - BatchRoutePutDedupDoesNotConsumeCapacity: a dedup hit consumes no projected capacity, so a same-size new key on a node with exactly one free slot still routes. - BatchRoutePutDoesNotWriteBackProjectedCapacity: batch-local capacity deductions are not written back to the store's client record. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…r-store tests to new API The rebase added a unique_ptr<MasterEvictStrategy> member to MasterServerConfig while the type is only forward-declared, so its implicit destructor failed to compile in every TU lacking evict_strategy.h — including the umbp_common and umbp_master libraries. Declare the special members out-of-line in config.h and define them = default in master_server.cpp (now includes evict_strategy.h). Port the three remaining tests to the policy-neutral store API EnumerateEvictionCandidates(buckets, EvictionOrder, max_per_bucket, now): - test_in_memory_master_metadata_store: Budget byte-map helper -> Buckets list; cap is now a count, not a byte budget (byte-budget trimming is the strategy's job); rename EvictionLruOrderAndBudget->AndCap, OnlyBudgetedBuckets->OnlyRequestedBuckets. - mock_master_metadata_store: swap EnumerateLruForEviction MOCK_METHOD for the 4-arg EnumerateEvictionCandidates; drop dead BudgetMap/LruResult aliases. - test_master_metadata_store_interface: update ON_CALL + the interface call. - test_master_evict_strategy: drop the deleted global_block_index.h include (EvictionCandidate/Location now live in types.h); Clock steady->system_clock to match EvictionCandidate::last_accessed_at. Builds clean and all umbp ctest pass (cross_node_smoke excluded; needs 2 nodes). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve the refactor-vs-perf collision: #440 optimized the legacy GlobalBlockIndex/ClientRegistry that this branch deletes in favor of IMasterMetadataStore. Keep the deletions and port #440's non-sharding wins onto the new store; defer block-index sharding to a follow-up so it stays separate from this conflict resolution. Applied from #440: - Add IMasterMetadataStore::GetAlivePeerView() (node->peer for ALIVE clients) + InMemory impl + GMock; router BatchRouteGet and master MatchExternalKv use it instead of copying full ClientRecords. - Keep the gRPC sync-server poller pool sizing (UMBP_MASTER_MIN/MAX_POLLERS). - Drive RemoveBlocksByNodeLocked off the node_to_keys_ reverse index (O(node keys) vs full scan); benefits Unregister/ExpireStaleClients. - Drop CLEAR_AT_TIER everywhere to match main's event protocol. - Keep kvevent master-pressure bench; drop the dead bench_global_block_index_route_get target (class removed). - Port the old ClientRegistry peer-view/count tests onto the store interface suite; test_peer_dram_allocator additions carried via rename. Deferred to a separate commit (A2): re-introduce per-shard locking for the block index inside InMemoryMasterMetadataStore. The single mutex_ (cross-store atomicity for the Redis/HA backend) is preserved unchanged; dropped the now-unimplemented UMBP_MASTER_INDEX_SHARDS doc entry.
Apply clang-format to the GetAlivePeerView impl and the ported peer-view store test (pre-commit clang-format hook).
clang-format (pre-commit --all-files, the config CI runs) reflows the EnumerateEvictionCandidates / SelectVictims calls and re-aligns the EvictionOrder enum comments. Pre-existing refactor-branch formatting the per-file hook run did not cover.
…hash Split InMemoryMasterMetadataStore into two lock domains: meta_mutex_ (clients + external-kv + hit counts) and N key-hashed block shards (UMBP_MASTER_INDEX_SHARDS, default 32, =1 = single lock), restoring PR #440's per-shard heartbeat concurrency the store merge had serialized behind one mutex. Cross-domain writes (ApplyHeartbeat / UnregisterClient / ExpireStaleClients) run meta first, then the block section after releasing meta_mutex_ — never nested, so heartbeats to different shards apply in parallel. Atomic per domain, not globally atomic; the reader windows are benign and match main/#440 (see the contract in master_metadata_store.h). Public API and event semantics unchanged. Re-adds the UMBP_MASTER_INDEX_SHARDS doc row and two concurrency conformance tests. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Technical Details
Test Plan
Test Result
Submission Checklist