Skip to content

impl(spanner): add request ID generation functions - #16465

Open
scotthart wants to merge 2 commits into
googleapis:mainfrom
scotthart:spanner_request_id_header_1
Open

scotthart wants to merge 2 commits into
googleapis:mainfrom
scotthart:spanner_request_id_header_1

Conversation

@scotthart

Copy link
Copy Markdown
Member

This PR adds inert code (to be used later) to generate and format Request ID headers for Spanner. Per the process specific requirements, there is some additional logic on non-Windows platforms to handle forking of processes.

@scotthart
scotthart requested a review from a team as a code owner September 20, 2026 15:50
@product-auto-label product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Sep 20, 2026

@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 introduces a new utility for generating and formatting Spanner request IDs, including its implementation, unit tests, and build configuration updates. The review feedback recommends addressing a potential fork deadlock risk and performance bottleneck in ProcessRandomId by replacing the std::mutex with a lock-free atomic pattern. Additionally, it suggests capturing and verifying the return value of the write system call in the unit tests to prevent compiler warnings under strict build settings.

Comment thread google/cloud/spanner/internal/spanner_request_id.cc Outdated
Comment thread google/cloud/spanner/internal/spanner_request_id_test.cc Outdated
Comment on lines +54 to +56
EXPECT_THAT(c1, Gt(0ULL));
EXPECT_THAT(c2, Eq(c1 + 1));
EXPECT_THAT(c3, Eq(c2 + 1));

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.

nit: why not check for Eq(1), Eq(2), Eq(3) ?

and monotonic would be more relaxed like:

  EXPECT_THAT(c2, Gt(c1));
  EXPECT_THAT(c3, Gt(c2));

@codecov

codecov Bot commented Sep 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.01299% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.34%. Comparing base (a892ceb) to head (bca26da).

Files with missing lines Patch % Lines
.../cloud/spanner/internal/spanner_request_id_test.cc 83.33% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #16465      +/-   ##
==========================================
+ Coverage   92.31%   92.34%   +0.02%     
==========================================
  Files        2246     2248       +2     
  Lines      214568   214645      +77     
==========================================
+ Hits       198086   198220     +134     
+ Misses      16482    16425      -57     

☔ 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

api: spanner Issues related to the Spanner API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants