Skip to content

impl(rest): add completion queue and thread types with no grpc deps#16078

Merged
scotthart merged 1 commit intogoogleapis:mainfrom
scotthart:pure_rest_cq_threads
Apr 15, 2026
Merged

impl(rest): add completion queue and thread types with no grpc deps#16078
scotthart merged 1 commit intogoogleapis:mainfrom
scotthart:pure_rest_cq_threads

Conversation

@scotthart
Copy link
Copy Markdown
Member

Upcoming OAuth features require timers and background processing. The existing REST completion queue and background thread types still depend on gRPC types from the original CompletionQueue making them unsuitable. If we had known a few years ago we'd need this, it would have been done differently then.

This PR adds the needed "pure" REST versions of background threads and completion queue. moves RunAsyncBase to its own file in the common library, and implements RestCompletionQueueImpl using the new RestPureCompletionQueueImpl which is still based on TimerQueue.

Hopefully we can reconcile these various types in the future and reduce the need for all the varieties.

@scotthart scotthart requested a review from a team as a code owner April 13, 2026 19:39
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

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 "pure REST" implementations for background threads and completion queues to decouple the REST library from gRPC dependencies. It refactors RestCompletionQueueImpl to use these new components and moves RunAsyncBase to a dedicated header. Feedback recommends adding a joinable check before joining threads in the background thread pool and removing an unused std::enable_shared_from_this inheritance to simplify the RestPureCompletionQueueImpl class.

Comment thread google/cloud/internal/rest_pure_background_threads_impl.cc
Comment thread google/cloud/internal/rest_pure_completion_queue_impl.h
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 13, 2026

Codecov Report

❌ Patch coverage is 36.36364% with 42 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.67%. Comparing base (efaacf4) to head (13e8938).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...loud/internal/rest_pure_background_threads_impl.cc 0.00% 30 Missing ⚠️
...e/cloud/internal/rest_pure_completion_queue_impl.h 42.85% 4 Missing ⚠️
.../cloud/internal/rest_pure_completion_queue_impl.cc 82.35% 3 Missing ⚠️
google/cloud/internal/rest_completion_queue_impl.h 33.33% 2 Missing ⚠️
...cloud/internal/rest_pure_background_threads_impl.h 0.00% 2 Missing ⚠️
...oogle/cloud/internal/rest_completion_queue_impl.cc 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #16078      +/-   ##
==========================================
- Coverage   92.69%   92.67%   -0.02%     
==========================================
  Files        2343     2348       +5     
  Lines      217439   217492      +53     
==========================================
+ Hits       201547   201555       +8     
- Misses      15892    15937      +45     

☔ View full report in Codecov by Sentry.
📢 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.

Copy link
Copy Markdown
Contributor

@jinseopkim0 jinseopkim0 left a comment

Choose a reason for hiding this comment

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

Would it make sense to add unit tests?

@scotthart
Copy link
Copy Markdown
Member Author

The implementation for PureRestCompletionQueue* was ported over from RestCompletionQueue* and then RestCompletionQueue* is implemented using PureRestCompletionQueue*. So, the existing unit tests for RestCompletionQueue* are testing PureRestCompletionQueue*.

@scotthart scotthart merged commit 7063180 into googleapis:main Apr 15, 2026
53 of 59 checks passed
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