Repository navigation
feat(spanner): add Request ID header support - #16448
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a generalized OperationContext mechanism to inject request IDs and track attempt lifecycles across retries, generalizing the Bigtable generator option and implementing a robust request ID tracking system in the Spanner client. The review feedback highlights critical thread-safety and correctness issues: a broken double-checked locking pattern in DefaultSpannerOperationContextFactory::StaticPrefix() that introduces data races, a thread-safety hazard in ProcessRandomId() due to returning a reference to a mutable static string instead of returning by value, and a bug in BatchWriteGuard where premature stream cancellation incorrectly reports a successful status.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16448 +/- ##
==========================================
- Coverage 92.34% 92.34% -0.01%
==========================================
Files 2254 2257 +3
Lines 215168 215559 +391
==========================================
+ Hits 198699 199058 +359
- Misses 16469 16501 +32 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
28e6fe0 to
779bb1f
Compare
779bb1f to
e72dda5
Compare
e72dda5 to
a4c94e3
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces SpannerOperationContextFactory and its default implementation to manage the creation of OperationContext instances across various Spanner operations, enabling better tracking of request IDs, channel IDs, and attempt counts. It updates ConnectionImpl, SessionPool, and associated tests and benchmarks to utilize this factory, and adds channel ID tracking to maintain channel affinity. Feedback on the changes highlights a potential null pointer dereference in the DefaultSpannerOperationContextFactory constructor if process_random_id is null, suggesting a defensive fallback to ProcessRandomId().
6269e2b to
ef1dddb
Compare
This PR adds the
spanner_internal::OperationContextFactoryand integrates the previous PRs to complete the Request ID header functionality.