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

Support remaining date based ChronoUnit enumerations for @Retry annotation #196

Closed
AlexXuChen opened this issue Oct 6, 2021 · 4 comments · Fixed by #197
Closed

Support remaining date based ChronoUnit enumerations for @Retry annotation #196

AlexXuChen opened this issue Oct 6, 2021 · 4 comments · Fixed by #197
Milestone

Comments

@AlexXuChen
Copy link
Contributor

As per #193 (comment):

We use java.time.Duration to calculate the difference between effective delay and max duration when passed as member values in the @Retry annotation. According to the implementation of Duration.plus, any date based ChronoUnit enumeration excluding DAYS (e.g. WEEKS, MONTHS, YEARS, etc..) will throw an UnsupportedTemporalTypeException.

Seeing that this is a limitation to the current implementation for @Retry diagnostics, (where we simply ignore these cases), there are 2 possible solutions:

  1. Have a wrapper to handle every date based case
  2. Wait for java.time.Duration to support date based enumeration(s)
@rgrunber
Copy link
Contributor

rgrunber commented Oct 6, 2021

Would this work ?

import java.time.temporal.ChronoUnit;
public double getDurationInNanos (ChronoUnit unit, long unitValue) {
  double seconds = (double) unit.getDuration().getSeconds();
  int nanos = unit.getDuration().getNano();
  return (seconds * 1000000000 * unitValue) + (nanos * unitValue);
}
jshell> getDurationInNanos(ChronoUnit.YEARS, 9) > getDurationInNanos(ChronoUnit.DECADES, 1)
$5 ==> false
jshell> getDurationInNanos(ChronoUnit.YEARS, 11) > getDurationInNanos(ChronoUnit.DECADES, 1)
$7 ==> true

@AlexXuChen
Copy link
Contributor Author

Would this work ?

Actually seems so! I assume this is because it doesn't use the Duration class.

@rgrunber
Copy link
Contributor

rgrunber commented Oct 6, 2021

We basically avoid any helper methods and just convert everything straight into the most common unit, nanoseconds.

There might be some optimization that could be done where we compare everything MILLIS or below in nanos and everything else in seconds. Ultimately that just saves us an extra 9 digits in case the input values get crazy, but hopefully we don't have to be that resilient for now.

@AlexXuChen
Copy link
Contributor Author

We also have https://docs.oracle.com/javase/8/docs/api/java/time/Duration.html#getNano--, but we may run into the same issue as before.

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 a pull request may close this issue.

2 participants