Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(common): add ability to supply a user-run CQ to gRPC options #7354

Merged
merged 2 commits into from
Sep 24, 2021

Conversation

dbolduc
Copy link
Member

@dbolduc dbolduc commented Sep 23, 2021

Fixes #7346

I went with GrpcCompletionQueueOption as the name.

I went with GrpcCompletionQueueOption > GrpcBackgroundThreadsFactoryOption > GrpcBackgroundThreadPoolSizeOption. (I put the CQ option first in case we ever deprecate the factory option.)

For testing the precedence, we have a > b > c so I just checked that a > b && b > c

I did not propagate the test for the option into things like Spanner, which tests other options from GrpcOptionList. We should think about how to test common options that show up in all of our libraries. Do we want to duplicate a common test N times? maybe.

Bigtable's client_options_test.cc should always have been using the MakeBackgroundThreadsFactory(opts) method instead of checking the option.


This change is Reviewable

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 23, 2021
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 30d8846a3c4649d3246250c976571afba885d4de

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 1eb28998ba275691e1f4bcf86189e141552cc33b

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@codecov
Copy link

codecov bot commented Sep 23, 2021

Codecov Report

Merging #7354 (1eb2899) into main (c541c55) will increase coverage by 0.01%.
The diff coverage is 95.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7354      +/-   ##
==========================================
+ Coverage   93.64%   93.65%   +0.01%     
==========================================
  Files        1349     1349              
  Lines      117050   117064      +14     
==========================================
+ Hits       109607   109635      +28     
+ Misses       7443     7429      -14     
Impacted Files Coverage Δ
.../bigtable/tests/instance_admin_integration_test.cc 3.76% <0.00%> (+0.02%) ⬆️
google/cloud/grpc_options_test.cc 89.61% <96.15%> (+1.32%) ⬆️
google/cloud/bigtable/benchmarks/benchmark.cc 87.54% <100.00%> (-0.05%) ⬇️
google/cloud/bigtable/client_options.cc 100.00% <100.00%> (ø)
google/cloud/bigtable/client_options_test.cc 99.74% <100.00%> (ø)
google/cloud/bigtable/instance_admin_test.cc 100.00% <100.00%> (ø)
...oud/bigtable/internal/async_longrunning_op_test.cc 100.00% <100.00%> (+0.78%) ⬆️
...ogle/cloud/bigtable/internal/async_poll_op_test.cc 98.91% <100.00%> (+1.04%) ⬆️
...le/internal/async_retry_unary_rpc_and_poll_test.cc 100.00% <100.00%> (+0.74%) ⬆️
google/cloud/bigtable/table_test.cc 100.00% <100.00%> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c541c55...1eb2899. Read the comment docs.

@dbolduc dbolduc marked this pull request as ready for review September 23, 2021 23:13
@dbolduc dbolduc requested a review from a team as a code owner September 23, 2021 23:13
@dbolduc dbolduc merged commit 1e37a02 into googleapis:main Sep 24, 2021
@dbolduc dbolduc deleted the grpc-cq-option branch September 24, 2021 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convenient way to pass a CQ to disable gRPC background threads using Options
4 participants