Add java.time.Duration overloads to CallOptions, AbstractStub, Deadline, and other APIs

[kluever]

Is your feature request related to a problem?

long, TimeUnit APIs encourage plumbing a pair of variables through various layers, or require plumbing a single ambiguous numeric primitive (e.g., long deadlineInSeconds). The modern alternative is to use a java.time.Duration.

Describe the solution you'd like

Every API that requests a long, TimeUnit should have a java.time.Duration overload as well

Describe alternatives you've considered

n/a

Additional context

n/a

[kluever]

Also leaving a breadcrumb that Eric mentioned to me: those methods will probably need the org.codehaus.mojo.animal_sniffer.IgnoreJRERequirement annotation on them.

[ejona86]

We still support non-javatime on Android, so these will need to be surface-level conveniences only. They should immediately convert to the current long-timeunit tuple methods.

[kluever]

SGTM --- this is very similar to what we do in Guava APIs that historically accept a long, TimeUnit tuple. E.g., https://github.com/google/guava/blob/master/guava/src/com/google/common/util/concurrent/Futures.java#L222-L259

You'll likely want to decompose to nanoseconds, but be aware of the potential for overflow (which happens at +/- 292 years). We've been using a helper function to decompose a Duration safely:
https://github.com/google/guava/blob/master/guava/src/com/google/common/util/concurrent/Internal.java#L27-L42

[ejona86]

TimeUnit.NANOSECONDS.convert(duration) looks better than Duration.toNano() for us, as it saturates and we aren't trying to avoid TimeUnit.

APIs that would be quick:
CallOptions
Deadline
AbstractStub

Then there's a longer tail:
ManagedChannelBuilder (and transport-specific friends)
ServerBuilder
... certainly there are others

Dunno about these and blocking methods in general. The only occurrence in java.util.concerrent is TimeUnit. I don't see a single blocking method have Duration in Java, so I think we'll leave the awaitTermination and blocking as old-style:
(don't change these)
ManagedChannel
Server

[kluever]

Thanks! My understanding is that the JDK folks have been a bit apprehensive about adding Duration-based overloads to java.util.concurrent APIs, since most of those APIs are very low-level concurrency building blocks (and allocation could matter in some cases). However, for user-facing APIs such as gRPC, Duration is almost certainly going to be better for your users.

[shubhamraj-samsung]

/assign

[shubhamraj-samsung]

@kluever @ejona86 Hi I am new to open source and would like to contribute to this issue. Please assign this issue to me

[ejona86]

Huh. I was able to assign you. I thought folks had to be in the grpc org for that to work.

[shubhamraj-samsung]

Hi @ejona86
I have implemented java.time.Duration overload function for CallOptions as follows. Please check if this is what you are expecting

   /**
   * Returns a new {@code CallOptions} with a deadline that is after the given {@code duration} from
   * now.
   */
    // This is my overloaded function with the **toNanosSaturated method** mentioned by @kluever 
  public CallOptions withDeadlineAfter(Duration duration) {
    return withDeadlineAfter(Deadline.after(toNanosSaturated(delay), TimeUnit.NANOSECONDS));
  }


  /**
   * Returns a new {@code CallOptions} with a deadline that is after the given {@code duration} from
   * now.
   */
  public CallOptions withDeadlineAfter(long duration, TimeUnit unit) {
    return withDeadline(Deadline.after(duration, unit));
  }

[shubhamraj-samsung]

My question is where should I create the java file for method toNanosSaturated(delay)
I thank you in advance for your patience:)

[kluever]

In Guava, we just made a package-private Internal.java class in each package where we wanted to use it: https://github.com/google/guava/blob/master/guava/src/com/google/common/util/concurrent/Internal.java#L27-L42

I'm not sure if gRPC can use Java 11+ APIs, but if so, you could do what @ejona86 suggested above: TimeUnit.convert(Duration)

[ejona86]

Doh, TimeUnit.convert(Duration) is Java 11. No, we can't use it. Yeah, just use a package-private class. I don't think we'd have a good class already to put it in; you can name it JavaTimeUtil or whatever.

[shubhamraj-samsung]

Cool. Thanks!
I got two more questions @ejona86 @kluever

1. grpc-java/api/src/main/java/io/grpc/SynchronizationContext.java

   Lines 177 to 179 in 4d3a29b

   	public final ScheduledHandle scheduleWithFixedDelay(
   	final Runnable task, long initialDelay, long delay, TimeUnit unit,
   	ScheduledExecutorService timerService) {

   In the above method there is requirement for initialDelay. How should I obtain it from java.time.Duration?

2. Should I raise the pull requests package wise (for example write the overloads for all files in API package and so on). That will be a little easier to review I guess. What do you think?

[kluever]

In scheduleWithFixedDelay(...), both delays use the same TimeUnit (which is a bit of a weird API, but that's how it works).

So I'd add an overload and have it's impl just delegate to the safe nano decomposition helper:

public final ScheduledHandle scheduleWithFixedDelay(Runnable task, Duration initialDelay, Duration delay, ScheduledExecutorService timerService) {
  return scheduleWithFixedDelay(
      task,
      toNanosSaturated(initialDelay),
      toNanosSaturated(delay),
      TimeUnit.NANOSECONDS,
      timerService);
}

[ejona86]

Sending a PR per-package or sets-of-classes is fine.

[ankit-kumar-dwivedi]

Hi @shubhamraj-samsung it's been a while since your last comment and I also don't see any related PR/draft.
Are you still working on this one?
If not @ejona86 can I pick this one?
Thanks!

[kannanjgithub]

TimeUnit.NANOSECONDS.convert(duration) looks better than Duration.toNano() for us, as it saturates and we aren't trying to avoid TimeUnit.

@ejona86 TimeUnit.NANOSECONDS.convert takes a long and a TimeUnit as arguments and returns the value in nanoseconds. java.time.Duration has getSeconds() so we could call TimeUnit.NANOSECONDS.convert(duration.getSeconds(), TimeUnit.SECONDS). But the nano portion from Duration will be lost.
Instead if we used duration.toNanos() directly there will be no loss of precision except in the case of overflow but the overflow is not peculiar to this approach but can occur in TimeUnit.NANOSECONDS.convert also.

[kluever]

Two slight corrections to the above comment:

• duration.toNanos() throws an exception if the duration is greater than 2^63 -1 nanoseconds
• TimeUnit conversion APIs won't overflow either, they silently saturate to Long.MAX_VALUE

[kannanjgithub]

About the 2nd point, yes, by overflow I meant the overflow and its handling in the exception block that we call saturation.

[SreeramdasLavanya]

@ejona86 During implementing java.time.Duration, I faced an issue with java version in build.gradle for the module grpc-api, to fix this issue we changed from VERSION_1_7 to VERSION_1_8 in my Local as java.time.Duration is part of Java 1.8 release. Please suggest

FYI below is the error message:
/usr/local/google/home/lsreeramdas/IdeaProjects/grpc-java-New/api/src/context/java/io/grpc/Deadline.java:19: error: package java.time does not exist
import java.time.Duration;

[ejona86]

We used to use -source and -target, which would let us use newer APIs (carefully). That's no longer the case with -release. Let's just not update Deadline for now.

[kannanjgithub]

@ejona86 can you also look at the concern I raised above about using the TimeUnit.NANOSECONDS.convert(long, TimeUnit) approach?

[SreeramdasLavanya]

We used to use -source and -target, which would let us use newer APIs (carefully). That's no longer the case with -release. Let's just not update Deadline for now.

Reverting changes for DeadLine and raising the PR for the same.

[ejona86]

@kannanjgithub wrote:

TimeUnit.NANOSECONDS.convert takes a long and a TimeUnit as arguments and returns the value in nanoseconds.

No, I was talking about the one that accepts a Duration directly. But then it was pointed out that method is Java 11+. So that meant we needed a helper to wrap duration.toNanos().

[SreeramdasLavanya]

@ejona86

I've done the necessary changes and raised PR, kindly confirm.

#11562