chore(spanner): integrate CacheSubscriber into DatabaseClient lifecycle - #6573
chore(spanner): integrate CacheSubscriber into DatabaseClient lifecycle#6573olavloite wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request integrates the background CacheSubscriber into the DatabaseClient lifecycle when location-aware routing is enabled, refactoring LocationRoutingState to manage the subscriber and delegating database ID retrieval to CacheUpdater. A critical memory visibility issue was identified where replacing the atomic load of database_id with CacheUpdater::database_id() downgrades the memory ordering from Acquire to Relaxed. This breaks release-acquire synchronization and could cause threads to read stale cache entries. It is recommended to update CacheUpdater::database_id() to use Ordering::Acquire to restore proper synchronization.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6573 +/- ##
=======================================
Coverage 96.59% 96.59%
=======================================
Files 304 304
Lines 91987 91909 -78
=======================================
- Hits 88855 88780 -75
+ Misses 3132 3129 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Initializes and starts the proactive background CacheSubscriber when location-aware routing is enabled on DatabaseClient. Ties background subscriber task termination to the RAII Drop lifecycle of LocationRoutingState, ensuring gRPC stream cleanup when all DatabaseClient clones are dropped.
3d3e00c to
84ad56d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request integrates the background CacheSubscriber into the DatabaseClient lifecycle when location-aware routing is enabled. It removes the separate database_id atomic field in LocationRoutingState in favor of retrieving it directly from CacheUpdater, which now uses Acquire ordering for loading the database ID. The PR also refactors mock tests to use a helper function create_test_mock to reduce boilerplate, and adds new lifecycle tests for the cache subscriber. There are no review comments, and the changes look solid, so I have no feedback to provide.
84ad56d to
570ef73
Compare
Initializes and starts the proactive background CacheSubscriber when location-aware routing is enabled on DatabaseClient. Ties background subscriber task termination to the RAII Drop lifecycle of LocationRoutingState, ensuring gRPC stream cleanup when all DatabaseClient clones are dropped.