-
Notifications
You must be signed in to change notification settings - Fork 61
feat: modernization part 2 #1748
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: main
Are you sure you want to change the base?
Conversation
|
Here is the summary of changes. You are about to delete 11 region tags.
This comment is generated by snippet-bot.
|
|
Warning: This pull request is touching the following templated files:
|
|
The CI is really not cooperating here (timeouts, quota errors, etc) but the code itself should be okay to review. |
mutianf
left a comment
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.
Where did all the data API examples go?
|
|
||
| // Imports the Admin library | ||
| const {BigtableTableAdminClient} = require('@google-cloud/bigtable').admin.v2; | ||
| const {TableAdminClient} = require('@google-cloud/bigtable').admin; |
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.
should this sample reflect the new CUJ?
await tableAdmin.waitForConsistency('tablename');
or
const [token] = await tableAdmin.generateConsistencyToken({
name: 'tablename',
});
// overload/etc, may be a separate method
await tableAdmin.waitForConsistency(token);
|
|
||
| // Instantiates a client | ||
| const adminClient = new BigtableTableAdminClient(); | ||
| const adminClient = new TableAdminClient(); |
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.
same here, should this be merged with check_consistency to reflect the newer way of generate consistency token and wait for table to be consistent?
|
|
||
| // Instantiates a client | ||
| const adminClient = new BigtableTableAdminClient(); | ||
| const adminClient = new TableAdminClient(); |
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.
This also doesn't reflect the new CUJ
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.
Important point for all of these under generated/, they are just GAPIC generated samples that got moved around and programmatically modified in owlbot.py to use the selective GAPIC class names and such. The admin ones we want to keep (although we could also just delete the restore table and consistency token ones if desired). The data plane ones, I talked with @igorbernstein2 about, we're just going to drop those to discourage their use vs the veneer.
I'm happy to do as you all like on these, though, I don't feel strongly about it.
| versions: 1, | ||
| }, | ||
| }; | ||
| const updatedMetadata = GcRuleBuilder.rule({ |
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.
I wonder if this example can be added to the modifyColumnFamilies example as well?
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.
I might've missed one, but I agree.
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.
Actually I'm not sure I see where it's missing - can you paste a link?
| * | ||
| * While users may create an instance of this class using the standard GAPIC | ||
| * constructor parameters, it's recommended to obtain one by way of the | ||
| * Bigtable.getTableAdminClient() method so that authentication and |
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.
I'm not sure if I understand this paragraph. In the example I see const adminClient = new TableAdminClient();. Is this section explaining how to get the TableAdminClient from a gapic generated client?
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 opposite, actually. It's explaining how to get a GAPIC client from the veneer object that users are used to using already. The GAPIC clients can be created without any extra parameters, so that's why the generated samples are like that, but if the user needs auth configuration or whatnot, it's simpler to get it from the Bigtable object than to rebuild it.
| * Please see the {@link https://github.com/googleapis/gax-nodejs/blob/master/client-libraries.md#long-running-operations | documentation } | ||
| * for more details and examples. | ||
| */ | ||
| async checkOptimizeRestoredTableProgress( |
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.
Do we have this in the example?
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.
Also this API looks a bit different from the doc? https://docs.google.com/document/d/14aOXOpEbwpYJe7-p6E-PUgoy0vn4AKI7zP0Man0prLo/edit?tab=t.0
| * @param options CallOptions, if desired | ||
| * @returns A Promise that completes when the table is consistent | ||
| */ | ||
| async waitForConsistency( |
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.
Do we have this in the example?
This is a follow-up for these two PRs:
#1739
#1740
This takes care of some missing pieces that were discovered by further discussion:
There is still a CI issue that will need to be corrected before this can be merged. I don't think it's the result of this change, but I also don't want to chance it.