[GitHub] [beam] mutianf commented on a diff in pull request #17823: [BEAM-14554] Create setting to report Bigtable client throttling time to Dataflow

[GitBox <gi...@apache.org>]

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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org