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.

Sign up for GitHub

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

Jump to bottom

feat(bigtable): per-operation Options (pt. 1) #9623

Merged
merged 2 commits into from
Aug 3, 2022
Merged

feat(bigtable): per-operation Options (pt. 1) #9623

merged 2 commits into from
Aug 3, 2022

Conversation

dbolduc
Copy link
Member

@dbolduc dbolduc commented Aug 2, 2022 •
edited
Loading

Most of the work for #7688

This PR handles all calls except for Table::(Async)ReadModifyWriteRow(...), which are a little bit more involved.

If Options are supplied to a Table implemented by DataClient, we will immediately return a kInvalidArgument error.

This change is Reviewable

@product-auto-label product-auto-label bot added the api: bigtable Issues related to the Bigtable API. label Aug 2, 2022
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 9bbccd93d46640f281afa4020fcfd8ee893554c9

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

@codecov
Copy link

codecov bot commented Aug 2, 2022 •
edited
Loading

Codecov Report

Merging #9623 (515c2d8) into main (919c183) will increase coverage by 0.00%.
The diff coverage is 98.50%.

@@           Coverage Diff            @@
##             main    #9623    +/-   ##
========================================
  Coverage   94.36%   94.36%            
========================================
  Files        1488     1488            
  Lines      137863   138017   +154     
========================================
+ Hits       130089   130246   +157     
+ Misses       7774     7771     -3
Impacted Files Coverage Δ
google/cloud/bigtable/data_connection.h 100.00% <ø> (ø)
google/cloud/bigtable/data_connection.cc 28.30% <50.00%> (+11.32%) ⬆️
google/cloud/bigtable/legacy_table_test.cc 100.00% <100.00%> (ø)
google/cloud/bigtable/table.cc 99.41% <100.00%> (+0.06%) ⬆️
google/cloud/bigtable/table.h 98.49% <100.00%> (+0.05%) ⬆️
google/cloud/bigtable/table_test.cc 100.00% <100.00%> (ø)
...e/cloud/spanner/testing/cleanup_stale_instances.cc 72.22% <0.00%> (-5.56%) ⬇️
...loud/bigtable/internal/connection_refresh_state.cc 95.83% <0.00%> (-2.78%) ⬇️
...cloud/pubsub/internal/subscription_session_test.cc 98.15% <0.00%> (-0.51%) ⬇️
google/cloud/storage/parallel_upload.cc 98.61% <0.00%> (+0.34%) ⬆️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@dbolduc dbolduc marked this pull request as ready for review August 3, 2022 01:16
@dbolduc dbolduc requested a review from a team as a code owner August 3, 2022 01:16
coryan
coryan previously requested changes Aug 3, 2022
View reviewed changes
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.

FWIW, I think there is some authoritative document describing what error code to use when, but I cannot recall its location right now (the gRPC github repo has a copy I think).

google/cloud/bigtable/table.cc Outdated
return connection_->Apply(table_name_, std::move(mut));
}
if (!google::cloud::internal::IsEmpty(opts)) {
return Status(StatusCode::kFailedPrecondition,
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think kFailedPrecondition is the right status code. kInvalidArgument seems more appropriate. kFailedPrecondition usually refers to a server-side pre-condition that was not met:

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, you're right. the docs say about kInvalidArgument: "Note that this differs from FAILED_PRECONDITION. INVALID_ARGUMENT indicates arguments that are problematic regardless of the state of the system (e.g., a malformed file name)."

Fixed.

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 515c2d89109e71f3b777e1e1154d6dcf99586c35

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

devbww
devbww approved these changes Aug 3, 2022
View reviewed changes
Copy link
Contributor

@devbww devbww left a comment

Choose a reason for hiding this comment

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

Don't forget to update the status code in the description.

@dbolduc dbolduc dismissed coryan’s stale review August 3, 2022 18:15

it has been addressed

@dbolduc dbolduc merged commit fe748ba into googleapis:main Aug 3, 2022
@dbolduc dbolduc deleted the bigtable-operation-options-pr-3 branch August 3, 2022 18:16
@dbolduc
Copy link
Member Author

dbolduc commented Aug 3, 2022

Don't forget to update the status code in the description.

Thanks. At no point did updating the PR description cross my mind.

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

@devbww devbww devbww approved these changes

@coryan coryan coryan left review comments

Assignees
No one assigned
Labels
api: bigtable Issues related to the Bigtable API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dbolduc @google-cloud-cpp-bot @coryan @devbww