igorbernstein2 commented on code in PR #24015:
URL: https://github.com/apache/beam/pull/24015#discussion_r1093438025
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableConfig.java:
##########
@@ -115,6 +120,11 @@ BigtableConfig withTableId(ValueProvider<String> tableId) {
return toBuilder().setTableId(tableId).build();
}
+ BigtableConfig withAppProfileId(@Nullable ValueProvider<String>
appProfileId) {
+ checkArgument(appProfileId != null, "App profile id can not be null");
+ return toBuilder().setAppProfileId(appProfileId).build();
+ }
+
/** @deprecated will be replaced by bigtable options configurator. */
@Deprecated
BigtableConfig withBigtableOptions(BigtableOptions options) {
Review Comment:
PLease deprecate the configurator for BigtbleOptions as well. We want to
move away from BigtableOptions entirely. Instead provide a different mechanism
for setting BigtableIO relevant options
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableConfig.java:
##########
@@ -196,66 +222,123 @@ BigtableService getBigtableService(PipelineOptions
pipelineOptions) {
return getBigtableService();
}
- BigtableOptions.Builder bigtableOptions =
effectiveUserProvidedBigtableOptions();
+ BigtableConfig.Builder config = toBuilder();
- bigtableOptions.setUserAgent(pipelineOptions.getUserAgent());
+ if (pipelineOptions instanceof GcpOptions) {
+ config.setCredentials(((GcpOptions) pipelineOptions).getGcpCredential());
+ }
- if (bigtableOptions.build().getCredentialOptions().getCredentialType()
- == CredentialOptions.CredentialType.DefaultCredentials) {
- bigtableOptions.setCredentialOptions(
-
CredentialOptions.credential(pipelineOptions.as(GcpOptions.class).getGcpCredential()));
+ try {
+ translateBigtableOptions(config);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
Review Comment:
Consider throwing a more specific error
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableConfig.java:
##########
@@ -196,66 +222,123 @@ BigtableService getBigtableService(PipelineOptions
pipelineOptions) {
return getBigtableService();
}
- BigtableOptions.Builder bigtableOptions =
effectiveUserProvidedBigtableOptions();
+ BigtableConfig.Builder config = toBuilder();
- bigtableOptions.setUserAgent(pipelineOptions.getUserAgent());
+ if (pipelineOptions instanceof GcpOptions) {
+ config.setCredentials(((GcpOptions) pipelineOptions).getGcpCredential());
+ }
- if (bigtableOptions.build().getCredentialOptions().getCredentialType()
- == CredentialOptions.CredentialType.DefaultCredentials) {
- bigtableOptions.setCredentialOptions(
-
CredentialOptions.credential(pipelineOptions.as(GcpOptions.class).getGcpCredential()));
+ try {
+ translateBigtableOptions(config);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
}
- return new BigtableServiceImpl(bigtableOptions.build());
+ config.setUserAgent(pipelineOptions.getUserAgent());
+
+ return new BigtableServiceImpl(config.build());
}
boolean isDataAccessible() {
- return getTableId().isAccessible()
- && (getProjectId() == null || getProjectId().isAccessible())
+ return (getProjectId() == null || getProjectId().isAccessible())
&& (getInstanceId() == null || getInstanceId().isAccessible());
}
- private BigtableOptions.Builder effectiveUserProvidedBigtableOptions() {
- BigtableOptions.Builder effectiveOptions =
- getBigtableOptions() != null
- ? getBigtableOptions().toBuilder()
- : new BigtableOptions.Builder();
+ private void translateBigtableOptions(BigtableConfig.Builder builder) throws
IOException {
+ BigtableOptions.Builder effectiveOptionsBuilder = null;
+
+ if (getBigtableOptions() != null) {
+ effectiveOptionsBuilder = getBigtableOptions().toBuilder();
+ }
if (getBigtableOptionsConfigurator() != null) {
- effectiveOptions =
getBigtableOptionsConfigurator().apply(effectiveOptions);
+ effectiveOptionsBuilder =
getBigtableOptionsConfigurator().apply(BigtableOptions.builder());
Review Comment:
I think the argument to the configurator should be `effectiveOptionsBuilder
|| BigtableOptions.builder()`
--
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]
