-
Notifications
You must be signed in to change notification settings - Fork 440
impl(v3): remove deprecated Bigtable admin classes #15920
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
base: prepare-for-v3.0.0
Are you sure you want to change the base?
impl(v3): remove deprecated Bigtable admin classes #15920
Conversation
Summary of ChangesHello @scotthart, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request completes the migration of Bigtable admin functionalities from legacy handwritten C++ classes to the modern, generated client libraries. This refactoring streamlines the codebase, removes technical debt, and ensures consistency with current API generation practices, providing a more maintainable and robust foundation for future development. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this 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 is a significant and well-executed effort to remove deprecated, handwritten Bigtable admin classes in favor of the modern, generated client APIs. The changes are extensive, touching documentation, build files, examples, and tests, and they correctly adapt the codebase to the new API patterns. My review focused on the correctness of this migration. I've identified a few opportunities to improve the migration guide to enhance clarity for users transitioning to the new APIs. Overall, this is a solid contribution that modernizes the Bigtable client library.
| google::bigtable::admin::v2::CheckConsistencyRequest wait_request; | ||
| wait_request.set_name(table_name); | ||
| wait_request.set_consistency_token(token->consistency_token()); | ||
| auto wait_response = table_admin.WaitForConsistency(wait_request).get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example for WaitForConsistency is a bit too brief. It retrieves the StatusOr from the future but doesn't demonstrate how to handle potential errors or use the result. Providing a more complete example would be beneficial for users migrating their code.
| auto wait_response = table_admin.WaitForConsistency(wait_request).get(); | |
| auto wait_future = table_admin.WaitForConsistency(wait_request); | |
| auto wait_response = wait_future.get(); | |
| if (!wait_response) throw std::runtime_error(wait_response.status().message()); | |
| // The table is now consistent. |
| **After:** | ||
|
|
||
| ```cpp | ||
| #include "google/cloud/bigtable/admin/bigtable_instance_admin_client.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example uses InstanceName, which is defined in google/cloud/bigtable/resource_names.h. This header should be included to ensure the code snippet is complete and compiles for users who copy it.
| #include "google/cloud/bigtable/admin/bigtable_instance_admin_client.h" | |
| #include "google/cloud/bigtable/admin/bigtable_instance_admin_client.h" | |
| #include "google/cloud/bigtable/resource_names.h" |
| auto clusters = instance_admin->ListClusters( | ||
| InstanceName("project-id", "instance-id")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example for ListClusters is a bit brief. Since ListClusters returns a StatusOr, it would be more helpful for users to see how to handle the status and iterate over the results.
| auto clusters = instance_admin->ListClusters( | |
| InstanceName("project-id", "instance-id")); | |
| auto clusters = instance_admin->ListClusters( | |
| InstanceName("project-id", "instance-id")); | |
| if (!clusters) throw std::runtime_error(clusters.status().message()); | |
| for (auto const& c : clusters->clusters()) { | |
| std::cout << c.name() << "\n"; | |
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## prepare-for-v3.0.0 #15920 +/- ##
======================================================
- Coverage 92.86% 92.79% -0.07%
======================================================
Files 2343 2332 -11
Lines 216218 214346 -1872
======================================================
- Hits 200795 198910 -1885
- Misses 15423 15436 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This PR removes the decommissioned handwritten admin APIs in favor of the generated APIs and updates the tests and samples.
The ABI dump and migration guide are also updated.