Skip to content

Conversation

@feywind
Copy link
Contributor

@feywind feywind commented Dec 11, 2025

This is a follow-up for these two PRs:

#1739
#1740

This takes care of some missing pieces that were discovered by further discussion:

  • GcRuleBuilder is a more TypeScript-heavy way to validate ModifyColumnFamilies garbage collection rules, with type checking taking care of a lot of the constraints, and some minimal runtime checking for JavaScript users.
  • Rather than raw GAPIC, the focus has shifted to selective GAPIC subclasses that can augment them. This is specifically done to assist with RestoreTable and waiting for table consistency.
  • Data plane samples have been removed, as we don't want to encourage users to use them.
  • Generated samples and tests have been auto-edited to use the selective GAPIC classes.
  • Manual sample snippets have been updated to reflect the expected admin access and GcRuleBuilder usage.

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.

@feywind feywind self-assigned this Dec 11, 2025
@feywind feywind requested review from a team as code owners December 11, 2025 21:57
@feywind feywind added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Dec 11, 2025
@snippet-bot
Copy link

snippet-bot bot commented Dec 11, 2025

Here is the summary of changes.

You are about to delete 11 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@product-auto-label product-auto-label bot added the size: xl Pull request size is extra large. label Dec 11, 2025
@generated-files-bot
Copy link

Warning: This pull request is touching the following templated files:

@product-auto-label product-auto-label bot added the api: bigtable Issues related to the googleapis/nodejs-bigtable API. label Dec 11, 2025
@feywind feywind changed the title Modernization part 2 feat: modernization part 2 Dec 11, 2025
@feywind feywind added owl-bot-copy owlbot:run Add this label to trigger the Owlbot post processor. labels Dec 11, 2025
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 11, 2025
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels Dec 11, 2025
@feywind feywind added the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 11, 2025
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 11, 2025
@feywind feywind added the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 12, 2025
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 12, 2025
@feywind feywind removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Dec 17, 2025
@feywind
Copy link
Contributor Author

feywind commented Dec 17, 2025

The CI is really not cooperating here (timeouts, quota errors, etc) but the code itself should be okay to review.

Copy link
Contributor

@mutianf mutianf left a 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;
Copy link
Contributor

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();
Copy link
Contributor

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();
Copy link
Contributor

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

Copy link
Contributor Author

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({
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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
Copy link
Contributor

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?

Copy link
Contributor Author

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(
Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

* @param options CallOptions, if desired
* @returns A Promise that completes when the table is consistent
*/
async waitForConsistency(
Copy link
Contributor

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?

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

Labels

api: bigtable Issues related to the googleapis/nodejs-bigtable API. size: l Pull request size is large.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants