Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accepting java.time.Duration as alternative to milliseconds in arguments #342

Open
aahlenst opened this issue May 23, 2023 · 9 comments
Open
Milestone

Comments

@aahlenst
Copy link
Contributor

It would be nice if all kinds of intervals and durations could be expressed as java.time.Duration in addition to a long of milliseconds. It would make it easier to understand code using Spring Retry when reading it because Duration.ofMinutes(5) is more obvious than 300_000 or 5*60*1000. Furthermore, the JDK has already adopted java.time.Duration for timeouts, for example, in Thread.sleep(Duration).

To clarify what I have in mind, an example involving RetryTemplateBuilder#fixedBackoff(long):

public RetryTemplateBuilder fixedBackoff(Duration interval) {
	Objects.requireNonNull(interval, "interval is null");
	return this.fixedBackoff(interval.toMillis());
}

Because it's a rather broad change, I'd like to know whether that's something you would accept and whether you rather want smaller steps (like changing RetryTemplateBuilder first, policies later) or everything in one PR.

@artembilan
Copy link
Member

This is rather too big and much involved.
More over it is going to be some kind of breaking change. We cannot have setters with the same name and different types: the XML parser does not understand which one to chose.
There is also no built-in support in Spring to parse duration strings automatically.

So, even if we accept this as feature improvement, it is not going to happen in the current version.
I would say that probably somewhere in 3.0.

If I would do this change, I would start from the bottom. For example, TimeoutRetryPolicy, then Sleeper and BackOff.

@snicoll
Copy link
Member

snicoll commented May 23, 2023

There is also no built-in support in Spring to parse duration strings automatically.

That's going to change in 6.1 hopefully but we need to have that as a base indeed before considering that change.

Can we maybe update the code that's not used by XML parsers for a start?

@artembilan
Copy link
Member

We cannot guess that because even simple <bean> for the mentioned TimeoutRetryPolicy would fail if we had setters like setTimeout(long) and setTimeout(Duration).
It would be a bit awkward to have a new one like setTimeoutDuration().
Replacing long with Duration will be a breaking change and that's why I call it 3.0 theme.
The mention builder in the beginning could probably introduce a Duration variants because its usage in the XML configuration is really hard.
But will it justify that change since we won't be able to apply such a Duration feature to target objects like UniformRandomBackOffPolicy or TimeoutRetryPolicy.

The @Backoff also have long attributes where for duration support we would need to change them to strings.

@snicoll
Copy link
Member

snicoll commented May 24, 2023

Can we maybe update the code that's not used by XML parsers for a start?

I meant code that's used programmatically (without setter like injection). Maybe we don't have a lot of those.

@aahlenst
Copy link
Contributor Author

Replacing long with Duration will be a breaking change and that's why I call it 3.0 theme.

I'm not sure if it is worth the effort and the upgrading pain because everyone has to change their code. I envisioned some overloads because that would save me a few keystrokes (Duration.ofMinutes(5).toMillis()) without breaking anyone's code or polluting the API (like setTimeoutDuration() would). But I wasn't aware that the XML configuration cannot distinguish overloads.

@artembilan
Copy link
Member

Yeah... So, let's start then from the RetryTemplateBuilder introducing new methods based on a Duration even if internally we still have to convert it back to the toMillis() to satisfy the other API which we cannot change at the moment.

@aahlenst
Copy link
Contributor Author

PR is up: #344

@artembilan artembilan added this to the Backlog milestone May 30, 2023
@quaff
Copy link
Contributor

quaff commented Aug 21, 2024

There is also no built-in support in Spring to parse duration strings automatically.

FYI, It's introduced in 6.2, see spring-projects/spring-framework#30396

@artembilan
Copy link
Member

Thank you for the pointer, @quaff .
Unfortunately we cannot upgrade there yet.
For that we would need a new version to start for this project.
We have some plans on the matter in the future anyway.

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

No branches or pull requests

4 participants