Skip to content

Conversation

lightoze
Copy link

No description provided.

@CLAassistant
Copy link

CLAassistant commented Jul 25, 2023

CLA assistant check
All committers have signed the CLA.

@@ -64,8 +64,11 @@ public static RetryOptions merge(MethodRetry r, RetryOptions o) {
} else {
builder.setBackoffCoefficient(DEFAULT_BACKOFF_COEFFICIENT);
}
int maximumAttempts = merge(r.maximumAttempts(), o.getMaximumAttempts(), int.class);
if (maximumAttempts != 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no sure if I follow that. How is zero different from null in the processing? Could you provide the code link?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shijiesheng I think the point is setMaximumAttempts method throws exception if the value is 0.
The value 0 is default and a valid value if "expiration" is set.
Unfortunately I cannot build the project locally because of thrift 0.9.3 not available on linux currently but test like that should show it:

public class RetryOptionsTest {
  @Test
  public void testUnlimitedRetryWithExpiration() throws NoSuchMethodException {
    RetryOptions topLevelOption =
        new RetryOptions.Builder().setExpiration(Duration.ofSeconds(5)).build();
    MethodRetry methodRetry =
        UnlimitedRetry.class.getMethod("retry").getAnnotation(MethodRetry.class);
    RetryOptions retryOptions = RetryOptions.merge(methodRetry, topLevelOption);
    Assert.assertEquals(
        "Top level options must take precedence", 5, retryOptions.getExpiration().getSeconds());
  }

  private interface UnlimitedRetry {
    @ActivityMethod
    @MethodRetry(expirationSeconds = 100)
    void retry();
  }
}

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1886

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 60.409%

Totals Coverage Status
Change from base Build 1863: 0.02%
Covered Lines: 11125
Relevant Lines: 18416

💛 - Coveralls

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants