[GitHub] [beam] aromanenko-dev commented on a change in pull request #11396: [BEAM-9742] Add Configurable FluentBackoff to JdbcIO Write

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

aromanenko-dev commented on a change in pull request #11396:
URL: https://github.com/apache/beam/pull/11396#discussion_r435399974



##########
File path: sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java
##########
@@ -1217,6 +1297,16 @@ void set(
       return toBuilder().setRetryStrategy(retryStrategy).build();
     }
 
+    /**
+     * When a SQL exception occurs, {@link Write} uses this {@link RetryConfiguration} to
+     * exponentially back off and retry the statements based on the {@link RetryConfiguration}
+     * mentioned.

Review comment:
       It would helpful to provide a simple example of using - here or in `JdbcIO` class Javadoc.

##########
File path: sdks/java/io/jdbc/src/main/java/org/apache/beam/sdk/io/jdbc/JdbcIO.java
##########
@@ -903,6 +907,72 @@ public void teardown() throws Exception {
     }
   }
 
+  /**
+   * Builder used to help with retry configuration for {@link JdbcIO}. The retry configuration
+   * accepts maxAttempts and maxDuration for {@link FluentBackoff}.
+   */
+  @AutoValue
+  public abstract static class RetryConfiguration implements Serializable {
+
+    abstract int getMaxAttempts();
+
+    @Nullable
+    abstract Duration getMaxDuration();
+
+    @Nullable
+    abstract Duration getInitialDuration();
+
+    abstract RetryConfiguration.Builder builder();
+
+    @AutoValue.Builder
+    abstract static class Builder {
+      abstract Builder setMaxAttempts(int maxAttempts);
+
+      abstract Builder setMaxDuration(Duration maxDuration);
+
+      abstract Builder setInitialDuration(Duration initialDuration);
+
+      abstract RetryConfiguration build();
+    }
+
+    public static RetryConfiguration create(int maxAttempts) {
+      checkArgument(maxAttempts > 0, "maxAttempts must be greater than 0");
+      return create(maxAttempts, DEFAULT_MAX_CUMULATIVE_BACKOFF, DEFAULT_INITIAL_BACKOFF);
+    }
+
+    public static RetryConfiguration create(int maxAttempts, Duration maxDuration) {
+      checkArgument(maxAttempts > 0, "maxAttempts must be greater than 0");
+      checkArgument(
+          maxDuration != null && maxDuration.isLongerThan(Duration.ZERO),
+          "maxDuration must be greater than 0");
+      return create(maxAttempts, maxDuration, DEFAULT_INITIAL_BACKOFF);
+    }
+
+    public static RetryConfiguration create(Duration initialDuration, int maxAttempts) {

Review comment:
       It's a bit confusing for user to have the overloaded methods where the same argument (eg, `maxAttempts`) changes its positions.  I'd prefer to leave only one method `create()` with all three arguments to provide - `create(int maxAttempts, Duration maxDuration, Duration initialDuration)`.
    
   Also, make `DEFAULT_INITIAL_BACKOFF ` and `DEFAULT_MAX_CUMULATIVE_BACKOFF` as the constants of `RetryConfiguration` and make them public.




----------------------------------------------------------------
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.

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