Abacn commented on code in PR #25955:
URL: https://github.com/apache/beam/pull/25955#discussion_r1149721899
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableServiceImpl.java:
##########
@@ -93,12 +93,32 @@ class BigtableServiceImpl implements BigtableService {
private static final long MIN_BYTE_BUFFER_SIZE = 100 * 1024 * 1024; // 100MB
public BigtableServiceImpl(BigtableDataSettings settings) throws IOException
{
- this.client = BigtableDataClient.create(settings);
+ this(settings, null);
+ }
+
+ public BigtableServiceImpl(BigtableDataSettings settings, Duration
readWaitTimeout)
+ throws IOException {
this.projectId = settings.getProjectId();
this.instanceId = settings.getInstanceId();
RetrySettings retry =
settings.getStubSettings().readRowsSettings().getRetrySettings();
this.readAttemptTimeout =
Duration.millis(retry.getInitialRpcTimeout().toMillis());
this.readOperationTimeout =
Duration.millis(retry.getTotalTimeout().toMillis());
+ BigtableDataSettings.Builder builder = settings.toBuilder();
+ if (readWaitTimeout != null) {
+ // TODO: a hack to workaround incorrect mapping from attempt timeout to
watchdogs wait
+ // timeout. This can be removed once
+ // https://github.com/googleapis/gapic-generator-java/pull/1473 is
resolved.
Review Comment:
This chunk should be done along with assembling BigtableDataSettings before
pass to the constructor of BigtableServiceImpl, then we do not need to add an
temporary (from my understanding to this TODO) argument to the public
BigtableServiceImpl constructor. Removing it later will be a breaking change.
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -306,7 +306,9 @@ public abstract static class Read extends
PTransform<PBegin, PCollection<Row>> {
/**
* Returns the Google Cloud Bigtable instance being read from, and other
parameters.
*
- * @deprecated please use {@link #getBigtableReadOptions()}.
+ * @deprecated please set the configurations directly:
+ *
BigtableIO.read().withProjectId(projectId).withInstanceId(instanceId).withTableId(tableId)
Review Comment:
What does this change mean? Is `getBigtableReadOptions()` no longer
recommended also? The original `deprecated` message was clear and now it seems
talking about another thing (set, instead of get configuration)
--
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]
