feat(bigquery-jdbc): add EnableProjectDiscovery connection property for metadata methods#13344
feat(bigquery-jdbc): add EnableProjectDiscovery connection property for metadata methods#13344keshavdandeva wants to merge 4 commits into
EnableProjectDiscovery connection property for metadata methods#13344Conversation
![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 introduces an EnableProjectDiscovery configuration property to automatically discover and list all accessible Google Cloud projects as catalogs. To support this, schema fetching in BigQueryDatabaseMetaData has been refactored to run concurrently using an executor service. Feedback on these changes highlights two main areas for improvement: first, replacing the fragile use of reflection to access the low-level BigQuery client with the standard public BigQuery.listProjects() API; second, ensuring that outstanding asynchronous tasks are properly cancelled if the schema fetching loop is interrupted to prevent resource leaks.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an automatic project discovery feature to the BigQuery JDBC driver, allowing users to discover all accessible Google Cloud projects via a new EnableProjectDiscovery connection property. It also updates BigQueryDatabaseMetaData to parallelize schema fetching across multiple projects. The review feedback highlights several areas for improvement: reusing a shared ExecutorService instead of creating a new one on every getSchemas() call, avoiding caching empty results on transient exceptions in getDiscoveredProjects(), using GsonFactory.getDefaultInstance() for better resource reuse, and preserving the stack trace when logging execution exceptions.
| } | ||
|
|
||
| try { | ||
| BigQueryOptions options = (BigQueryOptions) getBigQuery().getOptions(); |
There was a problem hiding this comment.
We have already code that provisions BigQuery client, we should not be creating new one (e.g. feels like HttpTransport is missing proxy/private endpoint and other properties that are configured).
Are there issues with SDK client?
There was a problem hiding this comment.
Yeah, so the SDK does not expose a public listProjects() method. To invoke the BigQuery REST /projects endpoint, we must drop down to the low-level client's projects().list() call.
I used reflection to extract the underlying client from BigQueryImpl but gemini said it is bad practice and is fragile and can throw dynamic access exceptions (e.g. InaccessibleObjectException under strict modular Java 17+ runtimes) or fail when BigQuery is mocked in tests.
Hence, the current implementation. And it should not bypass proxy, endpoint, or auth configurations.
- Proxy: Calling
transportOptions.getHttpTransportFactory().create()creates the exact HttpTransport configured in the connection provider (which contains any custom proxy factory settings). - Private Endpoints:
options.getResolvedApiaryHost(BIGQUERY_SERVICE_NAME)resolves the correct API host, incorporating user overrides or custom private service endpoints. - Auth/Timeouts/Headers:
transportOptions.getHttpRequestInitializer(options)automatically provisions the auth token initialization, user agent headers, and connection timeouts."
| private static final String BIGQUERY_SERVICE_NAME = "bigquery"; | ||
| private static final long MAX_PROJECTS_PER_PAGE = 10000L; | ||
| private static final String PROJECT_LIST_FIELDS = | ||
| "projects/projectReference/projectId,nextPageToken"; |
There was a problem hiding this comment.
Better to query projectName rather than projectId imo. This is used in some UI tools
There was a problem hiding this comment.
In the BigQuery REST API, there is no projectName field (only projectId and friendlyName). Using projectId is the standard way to reference GCP projects. Also, the catalog name returned by getCatalogs() must be the alphanumeric projectId.
| ExecutorService apiExecutor = null; | ||
| final List<Future<List<Dataset>>> apiFutures = new ArrayList<>(); | ||
| try { | ||
| apiExecutor = Executors.newFixedThreadPool(API_EXECUTOR_POOL_SIZE); |
There was a problem hiding this comment.
instead of create threadPool, we should have one available. We have MetaDataFetchThreadCount property available, but we actually lack connection-layer threadPool.
There was a problem hiding this comment.
Yes, completely agree. I noticed this as well and this is happening with all major metadata methods (getTables, getColumns, getProcedures, etc.). I created b/520400589 and will work on it in separate PR
| } | ||
|
|
||
| @Override | ||
| public ResultSet getSchemas(String catalog, String schemaPattern) { |
There was a problem hiding this comment.
I'd suggest some refactoring for this method, few ideas to simplify:
- Move code to fetch list of schemas in a specific catalog to a separate function;
- Keep this method simple - single catalog only, essentially calls into helper & transforms to jsonResultSet. No need for background threads since it is single catalog
- Refactor
getSchemas()to be the one that fans out multiple requests & assembles data.
Probably breaking up CLs in 2 will be easier:
- Add proper threadPool to connection-layer (and remove static threadPool in statement, it is not used)
- update getSchemas
Also I'd suggest to reuse more code. There are 4 metadata methods that can generate large # of rest calls:
- getCatalogs()
- getSchemas()
- getTables()
- getColumns()
- getProcedures()
In a way, they build results on top of each other. Right now we duplicate a lot, e.g. listDatasets() is called in 7 different places.
Also please don't tackle all of them in a single CL, lets do it incrementally :)
(Otherwise it is pain to review)
There was a problem hiding this comment.
Yeah, makes sense. I have created 3 bugs:
- b/520400589 - Refactor Metadata Thread Pool Management to Reuse Connection-Scoped Executor
- b/520407325 - Refactor getSchemas for Catalog-Based Routing (Synchronous & Async Fan-out)
- b/520406763 - Deduplicate metadata API calls
2 participants
b/499078725
This PR introduces the
EnableProjectDiscoveryconnection property (defaultfalse), which allows JDBC metadata methods to discover and query across all accessible Google Cloud projects, rather than being strictly limited to the project specified in the connection URL.Key Changes:
EnableProjectDiscoveryparameter parsing inBigQueryJdbcUrlUtilityandDataSource.BigQueryConnection.getDiscoveredProjects()to fetch all accessible GCP projects using the underlying low-levelBigqueryHTTP client.BigQueryDatabaseMetaData.getCatalogs()andgetSchemas()to return information across all discovered projects when the flag is enabled.getSchemas()using a fixed thread pool to significantly improve performance when scanning across multiple discovered projects.