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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
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.
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.
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:
|
| std::vector<std::string> names(list.size()); | ||
| std::transform(list.begin(), list.end(), names.begin(), | ||
| [](btadmin::AppProfile const& x) { return x.name(); }); | ||
| auto profile_names = [](StreamRange<btadmin::AppProfile>& list) |
There was a problem hiding this comment.
I'm curious why we went from a std::transform approach to a std::vector for-loop one.
There was a problem hiding this comment.
As we iterate though the StreamRange<T>, each element is actually a StatusOr<T>. So we need to check the Status of each element before we add it to the vector.
scotthart
merged commit 46847d2
into
googleapis:prepare-for-v3.0.0
2 participants
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.
Fixes #9584
This change is