Skip to content

Conversation

@pragnareddye
Copy link

What was changed

Why?

Setting setMaximumAttempts=0 (or not setting any value, default = 0) for local activities make the activity not retry at all.

There is no way to enable infinite retries for local activities

This is inconsistent with non-local/normal activity retry behavior

#1727

Checklist

  1. Closes
    inconsistency on MaximumAttempts attribute between local activities and "normal" activities #1727

  2. How was this tested:

Tested by running java samples with this change https://github.com/temporalio/samples-java

  1. Any docs updates needed?

Nope

@pragnareddye pragnareddye requested a review from a team as a code owner June 28, 2023 01:52
@CLAassistant
Copy link

CLAassistant commented Jun 28, 2023

CLA assistant check
All committers have signed the CLA.

@Quinn-With-Two-Ns
Copy link
Contributor

Hi @pragnareddye , thank you for the contribution. I need to go over this more tomorrow, but it would be helpful if we also add some test with this change to verify the local activity does retry infinitely until it times out.

@Quinn-With-Two-Ns
Copy link
Contributor

we should also verify

If both setScheduleToCloseTimeout(Duration) and RetryOptions.Builder.setMaximumAttempts(int) are not set, the Activity will not be retried.

@pragnareddye
Copy link
Author

Thank you so much for taking a look and pointing to the documentation requiring setScheduleToCloseTimeout(Duration) or setStartToCloseTimeout(Duration).

I was under the impression that the retry options set check wasn't necessary and that the expected behavior was that by default activities should retry indefinitely. This wasn't possible if scheduleToCloseTimeout was required. It is possible to infinitely retry with startToCloseTimeout set however. It's perhaps safer also and wouldn't require an update to the documentation.

I'll update the code first to fix it based on my new understanding and follow up with tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants