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

cleanup(bigtable)!: remove async functions that take CompletionQueue as a param #6921

Merged
merged 1 commit into from
Jul 1, 2021

Conversation

dbolduc
Copy link
Member

@dbolduc dbolduc commented Jul 1, 2021

Fixes #2567

Note: this leaves a lot of *Impl()'s that are called only once. I will refactor these in a subsequent PR.


This change is Reviewable

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

Google Cloud Build Logs
For commit: 6b2d219338949ff3e8d46fd3437a30ac8e1c16ef

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

identically named functions which do not require a `CompletionQueue&`. If
users absolutely require a custom `CompletionQueue`, one can be supplied to
the `DataClient` used to construct the `Table`, via
`ClientOptions::DisableBackgroundThreads(CompletionQueue const&)`.
Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Is this even worth saying?
  2. The change to prefer Options > ClientOptions will be done in this release. I am making a mental note to come back and update CHANGELOG.md accordingly

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, I think "You cannot use Foo() anymore, use Bar() instead" is more friendly/useful than "Foo() was removed."

It is also better to assume that folks have not memorized our reference manual, so they may not be familiar with every option in the library.

With those caveats, pointing folks to the recommended way to do things is the "Right Thing":tm:

Copy link
Contributor

Choose a reason for hiding this comment

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

The change is Okay as-is, we just need to update the CHANGELOG on the PRs where we start recommending Options{} instead.

@codecov
Copy link

codecov bot commented Jul 1, 2021

Codecov Report

Merging #6921 (6b2d219) into main (5db7a8c) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6921   +/-   ##
=======================================
  Coverage   94.55%   94.56%           
=======================================
  Files        1278     1278           
  Lines      114431   114421   -10     
=======================================
  Hits       108204   108204           
+ Misses       6227     6217   -10     
Impacted Files Coverage Δ
google/cloud/bigtable/table.cc 99.28% <ø> (+3.44%) ⬆️
google/cloud/bigtable/table.h 100.00% <ø> (ø)
google/cloud/bigtable/version.h 100.00% <ø> (ø)

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 5db7a8c...6b2d219. Read the comment docs.

@dbolduc dbolduc marked this pull request as ready for review July 1, 2021 21:36
@dbolduc dbolduc requested a review from a team as a code owner July 1, 2021 21:36
Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

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

Nice!

@dbolduc dbolduc merged commit 2214b8c into googleapis:main Jul 1, 2021
@dbolduc dbolduc deleted the bigtable-remove-async-fns-with-cq branch July 1, 2021 21:48
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.

Remove CompletionQueue parameter from Async*() functions.
4 participants