diff --git a/CHANGELOG.md b/CHANGELOG.md index af4bac6c19..1afe1bfd0b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,8 @@ to docs, or any other relevant information. specific to the Temporal CLI dev server implementation and may no longer be valid if that implementation changes. ### Fixed +- `RetryOptions.merge` now accepts partially configured policies, preserving unset initial intervals + and backoff coefficients until validation or annotation merging supplies them. - Test server now honors retry expiration deadlines that fall exactly on a whole second. Previously such deadlines were ignored and retries were scheduled past them instead of failing with `RETRY_STATE_TIMEOUT`. diff --git a/temporal-sdk/src/main/java/io/temporal/common/RetryOptions.java b/temporal-sdk/src/main/java/io/temporal/common/RetryOptions.java index 157354cc9c..f4ee0b5a18 100644 --- a/temporal-sdk/src/main/java/io/temporal/common/RetryOptions.java +++ b/temporal-sdk/src/main/java/io/temporal/common/RetryOptions.java @@ -77,18 +77,25 @@ public static RetryOptions merge(MethodRetry r, RetryOptions o) { .build(); } - /** The parameter options takes precedence. */ + /** The parameter options takes precedence. Unset fields remain unset until validation. */ public RetryOptions merge(RetryOptions o) { if (o == null) { return this; } - return RetryOptions.newBuilder() - .setInitialInterval( - OptionsUtils.merge(getInitialInterval(), o.getInitialInterval(), Duration.class)) + RetryOptions.Builder builder = RetryOptions.newBuilder(); + Duration initial = + OptionsUtils.merge(getInitialInterval(), o.getInitialInterval(), Duration.class); + if (initial != null) { + builder.setInitialInterval(initial); + } + double coefficient = + OptionsUtils.merge(getBackoffCoefficient(), o.getBackoffCoefficient(), double.class); + if (coefficient != 0d) { + builder.setBackoffCoefficient(coefficient); + } + return builder .setMaximumInterval( OptionsUtils.merge(getMaximumInterval(), o.getMaximumInterval(), Duration.class)) - .setBackoffCoefficient( - OptionsUtils.merge(getBackoffCoefficient(), o.getBackoffCoefficient(), double.class)) .setMaximumAttempts( OptionsUtils.merge(getMaximumAttempts(), o.getMaximumAttempts(), int.class)) .setDoNotRetry(OptionsUtils.merge(getDoNotRetry(), o.getDoNotRetry())) diff --git a/temporal-sdk/src/test/java/io/temporal/common/RetryOptionsTest.java b/temporal-sdk/src/test/java/io/temporal/common/RetryOptionsTest.java index ddb873e470..22f66bae4f 100644 --- a/temporal-sdk/src/test/java/io/temporal/common/RetryOptionsTest.java +++ b/temporal-sdk/src/test/java/io/temporal/common/RetryOptionsTest.java @@ -1,12 +1,92 @@ package io.temporal.common; +import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertSame; import java.time.Duration; import org.junit.Test; public class RetryOptionsTest { + @Test + public void mergeEmptyOptionsPreservesUnsetFields() { + RetryOptions merged = + RetryOptions.newBuilder().build().merge(RetryOptions.newBuilder().build()); + + assertNull(merged.getInitialInterval()); + assertEquals(0.0, merged.getBackoffCoefficient(), 0.0); + assertNull(merged.getMaximumInterval()); + assertNull(merged.getDoNotRetry()); + assertEquals(0, merged.getMaximumAttempts()); + assertEquals( + RetryOptions.newBuilder().validateBuildWithDefaults(), + merged.toBuilder().validateBuildWithDefaults()); + } + + @Test + public void mergePartialOptionsCombinesExplicitFields() { + RetryOptions original = RetryOptions.newBuilder().setMaximumAttempts(3).build(); + RetryOptions override = RetryOptions.newBuilder().setDoNotRetry("PermanentFailure").build(); + + RetryOptions merged = original.merge(override); + + assertEquals(3, merged.getMaximumAttempts()); + assertArrayEquals(new String[] {"PermanentFailure"}, merged.getDoNotRetry()); + assertNull(merged.getInitialInterval()); + assertEquals(0.0, merged.getBackoffCoefficient(), 0.0); + assertEquals( + Duration.ofSeconds(1), merged.toBuilder().validateBuildWithDefaults().getInitialInterval()); + } + + @Test + public void mergeInitialIntervalWithoutBackoff() { + RetryOptions original = + RetryOptions.newBuilder().setInitialInterval(Duration.ofSeconds(3)).build(); + RetryOptions merged = original.merge(RetryOptions.newBuilder().setMaximumAttempts(2).build()); + + assertEquals(Duration.ofSeconds(3), merged.getInitialInterval()); + assertEquals(0.0, merged.getBackoffCoefficient(), 0.0); + assertEquals(2.0, merged.toBuilder().validateBuildWithDefaults().getBackoffCoefficient(), 0.0); + } + + @Test + public void mergeExplicitBackoffWithoutInitialInterval() { + RetryOptions merged = + RetryOptions.newBuilder() + .setBackoffCoefficient(3.0) + .build() + .merge(RetryOptions.newBuilder().setBackoffCoefficient(4.0).build()); + + assertNull(merged.getInitialInterval()); + assertEquals(4.0, merged.getBackoffCoefficient(), 0.0); + } + + @Test + public void mergeExplicitFieldsPrefersOverride() { + RetryOptions original = + RetryOptions.newBuilder() + .setInitialInterval(Duration.ofSeconds(2)) + .setMaximumInterval(Duration.ofSeconds(20)) + .setBackoffCoefficient(3.0) + .setMaximumAttempts(5) + .setDoNotRetry("OriginalFailure") + .build(); + RetryOptions override = + RetryOptions.newBuilder() + .setInitialInterval(Duration.ofSeconds(4)) + .setMaximumInterval(Duration.ofSeconds(40)) + .setBackoffCoefficient(4.0) + .setMaximumAttempts(6) + .setDoNotRetry("OverrideFailure") + .build(); + + assertEquals(override, original.merge(override)); + assertEquals(original, original.merge(RetryOptions.newBuilder().build())); + assertSame(original, original.merge((RetryOptions) null)); + } + @Test public void mergePrefersTheParameter() { RetryOptions o1 = @@ -28,4 +108,26 @@ public void maximumIntervalCantBeLessThanInitial() { .setMaximumInterval(Duration.ofSeconds(1)) .validateBuildWithDefaults(); } + + @Test + public void mergePartialOptionsPreservesAnnotationValues() throws NoSuchMethodException { + MethodRetry annotation = + RetryOptionsTest.class.getMethod("annotatedRetry").getAnnotation(MethodRetry.class); + RetryOptions merged = + RetryOptions.newBuilder() + .setMaximumAttempts(3) + .build() + .merge(RetryOptions.newBuilder().setDoNotRetry("PermanentFailure").build()); + + RetryOptions resolved = + RetryOptions.merge(annotation, merged).toBuilder().validateBuildWithDefaults(); + + assertEquals(Duration.ofSeconds(5), resolved.getInitialInterval()); + assertEquals(4.0, resolved.getBackoffCoefficient(), 0.0); + assertEquals(3, resolved.getMaximumAttempts()); + assertArrayEquals(new String[] {"PermanentFailure"}, resolved.getDoNotRetry()); + } + + @MethodRetry(initialIntervalSeconds = 5, backoffCoefficient = 4.0, maximumAttempts = 10) + public void annotatedRetry() {} }