Skip to content

feat(bigtable): split gapic and proto usage from the handwritten library into bigtable-api - #8934

Draft
feywind wants to merge 17 commits into
mainfrom
feywind-split-bigtable
Draft

feat(bigtable): split gapic and proto usage from the handwritten library into bigtable-api#8934
feywind wants to merge 17 commits into
mainfrom
feywind-split-bigtable

Conversation

@feywind

@feywind feywind commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

This is in the effort to remove OwlBot from the standard repo path. A new package (bigtable-api) takes care of what OwlBot used to do (shuffle in new protos and generated gapic classes).

Internal: b/531788771

@product-auto-label product-auto-label Bot added the api: bigtable Issues related to the Bigtable API. label Jul 22, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

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 refactors the codebase to depend on the external @google-cloud/bigtable-api package for API clients and proto definitions, allowing the removal of numerous auto-generated client files, configurations, and shims. Feedback on the changes highlights a critical issue in handwritten/bigtable/src/admin/table.ts where JSON_PROTOS is read as a raw string using fs.readFileSync instead of being parsed as a JSON object, which will cause runtime failures in protobufFromJSON. A suggestion is provided to use require to synchronously load and parse the JSON file directly.

Comment thread handwritten/bigtable/src/admin/table.ts Outdated
@feywind
feywind marked this pull request as ready for review July 28, 2026 19:58
@feywind
feywind requested a review from a team as a code owner July 28, 2026 19:58
@feywind
feywind marked this pull request as draft July 28, 2026 22:20
@feywind

feywind commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

The PR might be a bit gross until I debug the resource leaks.

@quirogas quirogas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR looks good to me, Let's open follow up clean up bugs for:

  • removing proto files and addressing test_proxy.proto
  • cleanup tests, specially tests that depend on proto files
  • transition to the .json definition of the service file as opposed to using the proto files.
  • remove logs
  • split test changes into a separate PR.
  • remove utility script from publishing (if not already)

Feel free to reference this commnet to create the issues initially or create new ones altogether, up to you.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we delete this file or directory altogether and add the export logic to the index.ts file in the parent directory?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as admin/v2/index.ts. Could we move this export to the top-level file instead?

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 Bigtable API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants