Skip to content

[manager/meta] support V6D and subscriber reporting for multi-engine instances - #306

Open
lucky-zzz wants to merge 1 commit into
mainfrom
feature/multi_engine
Open

[manager/meta] support V6D and subscriber reporting for multi-engine instances#306
lucky-zzz wants to merge 1 commit into
mainfrom
feature/multi_engine

Conversation

@lucky-zzz

Copy link
Copy Markdown
Collaborator

Summary: support ranked reporter identities for multi-engine instances; project shared V6D cache locations onto active engine reporters while preserving the physical Vineyard peer; cover normal, non-shared V6D, and shared V6D reporting scenarios. Test: bazel test //kv_cache_manager/... (111 passed, 1 skipped).

@qoderai qoderai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Overall assessment: Solid, well-scoped change. The rank-suffix reporter identity, the shared-L2 → active-L1P5 projection, and the physical-vs-logical split for Vineyard peers are all implemented cleanly and covered by a comprehensive test. I don't see correctness bugs; the observations below are minor.

Lifetime / pointer safety

  • HostCacheLocationInfo::logical_hosts is a raw const std::vector<std::string> *, but it points into EventVisibilitySnapshot::logical_hosts_by_reporter, which lives inside the event_snapshots shared_ptr captured by the checker lambda. Since VisitHostSpecsForOneKey uses location_info only within the same callback invocation, the pointer is valid for its entire use. ✓
  • reporter_host is a std::string_view into the snapshot's location-id buffer; the snapshot is held alive by the same shared_ptr capture. ✓

ParseReporterIdentity

  • The rejection rules (empty rank, leading zero, duplicate @, non-digit, trailing chars) are all covered by the test cases @, @-1, @rank, @00, @0@1.
  • Using uint64_t for rank matches the rest of the codebase and avoids surprises for very large ranks.
  • One tiny nit: in the caller at cache_manager.cc:2705, ReporterIdentityView reporter_identity; is default-initialized and then passed by reference; since its value isn't used downstream of ReportEvent, consider dropping the local and passing an anonymous temporary, or at least marking it (void) to make the "validation-only" intent obvious to readers.

Projection logic (cache_manager.cc:4667-4702)

  • Building ranked_hosts_by_base from L1P5 and then filling logical_hosts_by_reporter per storage type is correct. The L2-only projection condition (parsed && storage_type == L2 && !identity.engine_rank.has_value()) matches the design: only a shared, unranked L2 reporter fans out to every active ranked engine.
  • The sort + unique on ranked_hosts is defensive; ranked_hosts_by_base is built by iterating each reporter once, so duplicates aren't expected in normal operation. Fine as-is.
  • Style nit: in for (auto &[base, ranked_hosts] : ranked_hosts_by_base) { (void)base; ... }, the binding could be const auto &[base, ranked_hosts] (and drop the (void)base) since neither is mutated, or just use auto &[_, ranked_hosts] if the toolchain supports C++26 structured bindings with _. Same for the (void)state; silencers.

Visitor signature change (meta_searcher.cc)

  • Adding a fourth physical_reporter argument and keying vineyard_host_specs by physical_reporter is the correct move: a shared L2 reporter projected onto N logical ranks must still yield exactly one Vineyard P2P peer, not N. The new comment at meta_searcher.cc:626-628 explains this well.
  • For non-event-report locations, the caller passes std::string_view(host) twice (logical and physical), which preserves prior behavior. ✓
  • All four existing callers (BuildHostsForOneKey, BuildCandidatePresenceForOneKey, BuildHostSpecNamesForOneKey, PrefixMatchWithMambaByHostWithoutP2P) are updated; VisitHostSpecsForOneKey is a file-local template so no out-of-TU breakage is possible. ✓

Tests

  • Three cases (ordinary / multi-engine independent / multi-engine shared) plus invalid-reporter coverage and a rank-lifecycle sequence. The GetCacheLocationsByBackend assertion in Case 3 is the right check that the data-access path does not get rewritten by the query-only projection — good regression guard.
  • One optional gap: no test exercises L1P5 shared (unranked) with ranked L2. That configuration isn't part of the design (the asymmetry is intentional: L1P5 = subscriber identity, L2 = shared Vineyard), so this is more of a "nice to have" negative test to lock in that no projection happens in the reverse direction.

Non-blocking observations

  • host_ip_port is reused as both a location-id component and a reporter identity. The @ character is accepted by SnapshotUriUtils::IsValidLocationIdComponent (otherwise the parse wouldn't see ranked input), worth a quick mental check that no downstream URI/snapshot builder chokes on it. The test URIs use event_report://host:port/mem without @, so the location URIs themselves stay clean.
  • The PR description says "111 passed, 1 skipped"; consider running the integration test suite too (//integration_test/...) if not already done, since meta_searcher.cc is exercised by cross-module scenarios.

No blocking issues; feel free to merge once the (optional) nits are considered.


🤖 Generated by Qoder

@github-actions github-actions Bot added the ai reviewed AI has reviewed this PR label Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai reviewed AI has reviewed this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant