gemini-code-assist[bot] commented on code in PR #39004:
URL: https://github.com/apache/beam/pull/39004#discussion_r3429587929
##########
sdks/java/ml/inference/remote/src/main/java/org/apache/beam/sdk/ml/inference/remote/RetryHandler.java:
##########
@@ -72,6 +89,11 @@ public <T> T execute(RetryableRequest<T> request) throws
Exception {
} catch (Exception e) {
lastException = e;
+ if (!retryFilter.shouldRetry(e)) {
+ LOG.warn("Exception not eligible for retry. Failing immediately.",
e);
+ throw e;
+ }
Review Comment:
Since `RetryHandler` implements `Serializable`, instances serialized with
older versions of this class will not have the `retryFilter` field populated
upon deserialization, resulting in `retryFilter` being `null`. To ensure
backward compatibility and prevent a `NullPointerException`, we should add a
null check before calling `shouldRetry`.
```suggestion
if (retryFilter != null && !retryFilter.shouldRetry(e)) {
LOG.warn("Exception not eligible for retry. Failing immediately.",
e);
throw e;
}
```
##########
sdks/java/ml/inference/remote/src/main/java/org/apache/beam/sdk/ml/inference/remote/RetryHandler.java:
##########
@@ -34,24 +34,41 @@ public class RetryHandler implements Serializable {
private final Duration initialBackoff;
private final Duration maxBackoff;
private final Duration maxCumulativeBackoff;
+ private final RetryFilter retryFilter;
+
+ @FunctionalInterface
+ public interface RetryFilter extends Serializable {
+ boolean shouldRetry(Exception e);
+ }
private RetryHandler(
- int maxRetries, Duration initialBackoff, Duration maxBackoff, Duration
maxCumulativeBackoff) {
+ int maxRetries,
+ Duration initialBackoff,
+ Duration maxBackoff,
+ Duration maxCumulativeBackoff,
+ RetryFilter retryFilter) {
this.maxRetries = maxRetries;
this.initialBackoff = initialBackoff;
this.maxBackoff = maxBackoff;
this.maxCumulativeBackoff = maxCumulativeBackoff;
+ this.retryFilter = retryFilter;
Review Comment:
To prevent `NullPointerException` at runtime, we should validate that
`retryFilter` is not null when constructing the `RetryHandler`. Using
`java.util.Objects.requireNonNull` ensures a fail-fast behavior if a null
filter is provided.
```suggestion
this.retryFilter = java.util.Objects.requireNonNull(retryFilter,
"retryFilter must not be null");
```
##########
sdks/java/ml/inference/remote/src/main/java/org/apache/beam/sdk/ml/inference/remote/RetryHandler.java:
##########
@@ -34,24 +34,41 @@ public class RetryHandler implements Serializable {
private final Duration initialBackoff;
private final Duration maxBackoff;
private final Duration maxCumulativeBackoff;
+ private final RetryFilter retryFilter;
+
+ @FunctionalInterface
+ public interface RetryFilter extends Serializable {
+ boolean shouldRetry(Exception e);
+ }
private RetryHandler(
- int maxRetries, Duration initialBackoff, Duration maxBackoff, Duration
maxCumulativeBackoff) {
+ int maxRetries,
+ Duration initialBackoff,
+ Duration maxBackoff,
+ Duration maxCumulativeBackoff,
+ RetryFilter retryFilter) {
this.maxRetries = maxRetries;
this.initialBackoff = initialBackoff;
this.maxBackoff = maxBackoff;
this.maxCumulativeBackoff = maxCumulativeBackoff;
+ this.retryFilter = retryFilter;
}
public static RetryHandler withDefaults() {
return new RetryHandler(
3, // maxRetries
Duration.standardSeconds(1), // initialBackoff
Duration.standardSeconds(10), // maxBackoff per retry
- Duration.standardMinutes(1) // maxCumulativeBackoff
+ Duration.standardMinutes(1), // maxCumulativeBackoff
+ (RetryFilter) e -> true // retryFilter default to retrying all
exceptions
Review Comment:
The explicit cast to `(RetryFilter)` is redundant because the compiler can
automatically infer the functional interface type from the constructor's
parameter signature. Removing it makes the code cleaner.
```suggestion
e -> true // retryFilter default to retrying all exceptions
```
--
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]
