[service] allow follower ReportEvent queries - #263
Conversation
There was a problem hiding this comment.
Review Summary
Minimal, correct change. Flipping GetHostCacheState's API_CALL_GUARD to is_leader_only=false is the right mechanism and has precedent in GetClusterInfo (line 825). ReportEvent correctly stays leader-only, so mutations aren't opened up to followers.
The regression test is well-constructed: using an empty instance_id to prove the follower gate was passed (reaching normal INVALID_ARGUMENT validation) rather than SERVER_NOT_LEADER is a neat way to assert gate behavior without standing up a full instance/data set, and the write-rejection assertion confirms the asymmetry. The -fno-access-control copt and deps match the file's existing conventions, and ServiceCallGuard is null-safe for the nullptr metrics_reporter (guarded at service_call_guard.cc:142).
One inline note about a behavioral consequence worth confirming: followers now surface INSTANCE_NOT_EXIST (rather than SERVER_NOT_LEADER) for instances not in their local view, and it's worth confirming (a) the client treats non-leader responses as non-authoritative, and (b) that followers actually hold a populated local view given ReportEvent is leader-only.
No blocking issues.
🤖 Generated by Qoder
| // This is a best-effort read of the node's local ReportEvent view. Followers may | ||
| // serve a stale view, while ReportEvent mutations remain leader-only. | ||
| API_CALL_GUARD("GetHostCacheState", false); |
There was a problem hiding this comment.
Allowing followers to serve this read is a reasonable best-effort behavior, and is_leader_only=false mirrors GetClusterInfo (line 825). Two consequences worth confirming:
-
Error-code shift for missing instances. Previously a follower always returned
SERVER_NOT_LEADERhere regardless of the instance. Now a follower whose local view doesn't contain the instance returnsINSTANCE_NOT_EXIST(viaCacheManager::GetHostCacheState→meta_searcher_manager_->GetMetaSearcher, cache_manager.cc:3703, mapped throughToMetaPbError). Callers that readINSTANCE_NOT_EXISTas "instance doesn't exist anywhere" rather than "this follower doesn't have it locally" could behave incorrectly — worth confirming the client SDK retries the leader (or otherwise treats non-leader responses as non-authoritative) on this code. -
Is the follower's local view actually populated?
ReportEvent(the write path that populates this state) is leader-only, so a node that has never been leader won't have received those mutations. Is the meta/registry state replicated to followers through another path, or is the expected local view limited to a stale snapshot from when this node was last leader? The latter is fine for best-effort reads — just want it to be explicit sinceOnNoLongerLeaderpauses the reclaimer but doesn't otherwise sync new events.
🤖 Generated by Qoder
Summary
GetHostCacheStateto serve follower requests as a best-effort read of the node's local ReportEvent view.ReportEventmutations leader-only.Motivation
When the leader is unavailable, followers should still be able to return their local cache state. The returned state may be stale and is not intended to provide strong consistency.
Validation
//kv_cache_manager/service/test:allwith debug + ASAN — 7/7 passed.//kv_cache_manager/service/test:allwith debug + ASAN — 7/7 passed.git diff --checkbuildifier -mode=check kv_cache_manager/service/test/BUILD