mutianf commented on code in PR #17823:
URL: https://github.com/apache/beam/pull/17823#discussion_r890484611
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableConfig.java:
##########
@@ -97,6 +100,8 @@ abstract Builder setBigtableOptionsConfigurator(
abstract Builder setEmulatorHost(String emulatorHost);
+ abstract Builder setBulkMutationDataflowThrottling(boolean isEnabled);
Review Comment:
I think the name is a bit too long, maybe change it to
`setBulkMutationThrottling`.
Also add `@Experimental` flag, and add a comment about this is the
experimental feature
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableConfig.java:
##########
@@ -144,6 +149,11 @@ BigtableConfig withEmulator(String emulatorHost) {
return toBuilder().setEmulatorHost(emulatorHost).build();
}
+ @VisibleForTesting
+ BigtableConfig withBulkMutationDataflowThrottling(boolean isEnabled) {
Review Comment:
withBulkMutationThrottling and `@Experimental`
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableConfig.java:
##########
@@ -69,6 +69,9 @@ abstract class BigtableConfig implements Serializable {
/** Bigtable emulator. Used only for testing. */
abstract @Nullable String getEmulatorHost();
+ /** Reporting throttle time to Dataflow flag. */
+ abstract boolean getBulkMutationDataflowThrottling();
Review Comment:
I think the name is a bit too long, maybe change it to
`getBulkMutationThrottling` and add an `@Experimental` flag and a comment
saying it's experimental feature.
And also comment should be something like: whether throttling time is
reported to dataflow jobs
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIOTest.java:
##########
@@ -324,7 +333,11 @@ public void testReadWithReaderAdvanceFailed() throws
IOException {
FailureBigtableService failureService =
new
FailureBigtableService(FailureOptions.builder().setFailAtAdvance(true).build());
BigtableConfig failureConfig =
-
BigtableConfig.builder().setValidate(true).setBigtableService(failureService).build();
+ BigtableConfig.builder()
+ .setValidate(true)
+ .setBulkMutationDataflowThrottling(true)
Review Comment:
Should this be false?
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIOTest.java:
##########
@@ -796,7 +809,11 @@ public void testReadingWithSplitFailed() throws Exception {
FailureBigtableService failureService =
new
FailureBigtableService(FailureOptions.builder().setFailAtSplit(true).build());
BigtableConfig failureConfig =
-
BigtableConfig.builder().setValidate(true).setBigtableService(failureService).build();
+ BigtableConfig.builder()
+ .setValidate(true)
+ .setBulkMutationDataflowThrottling(true)
Review Comment:
False here too
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -563,13 +573,23 @@ public final String toString() {
return getBigtableConfig().getBigtableOptions();
}
+ /**
+ * Returns whether client's throttle time is being passed to Dataflow for
bulk mutations
+ *
+ * <p>This change is experimental and may be changed and relocated in the
future
+ */
+ public boolean isBulkMutationDataflowThrottlingEnabled() {
Review Comment:
`@Experimental`
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIOTest.java:
##########
@@ -302,7 +307,11 @@ public void testReadWithReaderStartFailed() throws
IOException {
FailureBigtableService failureService =
new
FailureBigtableService(FailureOptions.builder().setFailAtStart(true).build());
BigtableConfig failureConfig =
-
BigtableConfig.builder().setValidate(true).setBigtableService(failureService).build();
+ BigtableConfig.builder()
+ .setValidate(true)
+ .setBulkMutationDataflowThrottling(true)
Review Comment:
Should this be false?
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -739,6 +759,15 @@ public Write withEmulator(String emulatorHost) {
return
toBuilder().setBigtableConfig(config.withEmulator(emulatorHost)).build();
}
+ /* This is an experimental feature that may get changed in the future
+ *
+ * Returns a new {@link BigtableIO.Write} that will report amount of time
throttling to Dataflow
+ */
+ public Write withBulkMutationDataflowThrottling() {
Review Comment:
`@Experimental`
--
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]
