joyhaldar commented on code in PR #14447: URL: https://github.com/apache/iceberg/pull/14447#discussion_r2523455045
########## bigquery/src/main/java/org/apache/iceberg/gcp/bigquery/BigQueryMetastoreCatalog.java: ########## @@ -63,6 +62,11 @@ public class BigQueryMetastoreCatalog extends BaseMetastoreCatalog public static final String GCP_LOCATION = "gcp.bigquery.location"; public static final String LIST_ALL_TABLES = "gcp.bigquery.list-all-tables"; + public static final String CLIENT_FACTORY = "gcp.bigquery.client.factory"; + private static final String GCS_IMPERSONATE_SERVICE_ACCOUNT = "gcs.impersonate.service-account"; + private static final String GCS_PROJECT_ID = "gcs.project-id"; Review Comment: Thank you so much for reviewing @rambleraptor. I wanted to explain the flow a bit: - [ImpersonatedBigQueryClientFactory](https://github.com/apache/iceberg/blob/81607aa4d1719f07f003100955d0bfe9863a57e9/bigquery/src/main/java/org/apache/iceberg/gcp/bigquery/ImpersonatedBigQueryClientFactory.java) defines: `gcp.bigquery.impersonate.service-account` (used for BigQuery client) - [prepareFileIOProperties()](https://github.com/apache/iceberg/blob/81607aa4d1719f07f003100955d0bfe9863a57e9/bigquery/src/main/java/org/apache/iceberg/gcp/bigquery/BigQueryMetastoreCatalog.java#L167) transforms: `gcp.*` → `gcs.*` (for FileIO, based on constants defined in [GCPProperties](https://github.com/apache/iceberg/blob/81607aa4d1719f07f003100955d0bfe9863a57e9/gcp/src/main/java/org/apache/iceberg/gcp/GCPProperties.java#L53)) - GCPProperties reads: `gcs.impersonate.service-account` Do you think this should be changed? I can try removing the private constants from BigQueryMetastoreCatalog and use GCPProperties.GCS_* directly. Please let me know what you think would be more appropriate. ########## bigquery/src/main/java/org/apache/iceberg/gcp/bigquery/BigQueryMetastoreClientImpl.java: ########## @@ -127,9 +128,21 @@ public BigQueryMetastoreClientImpl(BigQueryOptions options) throws IOException, GeneralSecurityException { // Initialize client that will be used to send requests. This client only needs to be created // once, and can be reused for multiple requests - HttpCredentialsAdapter httpCredentialsAdapter = - new HttpCredentialsAdapter( - GoogleCredentials.getApplicationDefault().createScoped(BigqueryScopes.all())); + + // Get credentials from options, or use application default + GoogleCredentials credentials = + (options.getCredentials() instanceof GoogleCredentials) + ? (GoogleCredentials) options.getCredentials() + : GoogleCredentials.getApplicationDefault(); + + // Scope credentials unless already scoped (e.g., ImpersonatedCredentials) + GoogleCredentials scopedCredentials = + (credentials instanceof ImpersonatedCredentials) + ? credentials + : credentials.createScoped(BigqueryScopes.all()); Review Comment: I would also live Talat's opinion on this. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
