Conversation
There was a problem hiding this comment.
Code Review
This pull request generalizes the experimental OperationContext feature from being Bigtable-specific to a reusable component across other services. It introduces a common abstract base class google::cloud::internal::OperationContext in the core gRPC utilities and updates the generator to dynamically use the product's internal namespace instead of hardcoding Bigtable. Consequently, Bigtable's OperationContext now inherits from this base class, and various stub decorators are updated to use bigtable_internal::OperationContext. The review feedback focuses on minimizing header coupling and reducing compilation times by preferring forward declarations of OperationContext in stub headers instead of directly including the operation_context.h header. Additionally, a defensive check is recommended in the generator's descriptor utilities to safely look up the product_internal_namespace key and prevent potential crashes if it is missing.
| #include "google/cloud/bigtable/internal/operation_context.h" | ||
| #include "google/cloud/async_streaming_read_write_rpc.h" |
There was a problem hiding this comment.
To keep compilation times low and minimize header coupling, we should prefer forward declaring OperationContext in bigtable_stub.h instead of including its header. Since BigtableStub only uses OperationContext by reference (OperationContext&) and std::shared_ptr, a forward declaration is fully sufficient for this header file. This avoids transitively including <grpcpp/grpcpp.h> and <map> in all files that include bigtable_stub.h.
#include "google/cloud/async_streaming_read_write_rpc.h"References
- Prefer including headers directly in .cc files rather than .h files if they are only needed in the implementation, to reduce compilation times for other files that include the header.
There was a problem hiding this comment.
Keeping the include https://google.github.io/styleguide/cppguide.html#Forward_Declarations
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16464 +/- ##
==========================================
+ Coverage 92.31% 92.34% +0.02%
==========================================
Files 2246 2247 +1
Lines 214568 214572 +4
==========================================
+ Hits 198086 198152 +66
+ Misses 16482 16420 -62 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
In preparation for some Spanner work and wider use of OperationContext, this PR defines the OperationContext interface and refactors bigtable and the generator to use it.
No logic or behavior changes are introduced in this PR.