Skip to content

refactor: add OperationContext base class and refactor bigtable - #16464

Open
scotthart wants to merge 3 commits into
googleapis:mainfrom
scotthart:add-internal-operation-context
Open

scotthart wants to merge 3 commits into
googleapis:mainfrom
scotthart:add-internal-operation-context

Conversation

@scotthart

Copy link
Copy Markdown
Member

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.

@scotthart
scotthart requested a review from a team as a code owner September 20, 2026 15:33

@gemini-code-assist gemini-code-assist 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.

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.

Comment on lines +22 to 23
#include "google/cloud/bigtable/internal/operation_context.h"
#include "google/cloud/async_streaming_read_write_rpc.h"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

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
  1. 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Comment thread google/cloud/bigtable/internal/bigtable_stub.h
Comment thread generator/internal/stub_generator.cc
Comment thread generator/internal/stub_generator.cc
Comment thread generator/internal/descriptor_utils.cc

@dbolduc dbolduc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Neat!

@codecov

codecov Bot commented Sep 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 63.15789% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.34%. Comparing base (a892ceb) to head (1d399c2).

Files with missing lines Patch % Lines
generator/internal/descriptor_utils.cc 22.22% 7 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants