grpc-java
grpc-java copied to clipboard
grpc
Add java.time.Duration overloads to CallOptions, AbstractStub, Deadline, and other APIs
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
Also leaving a breadcrumb that Eric mentioned to me: those methods will probably need the org.codehaus.mojo.animal_sniffer.IgnoreJRERequirement annotation on them.
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.
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
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
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.
@kluever @ejona86 Hi I am new to open source and would like to contribute to this issue. Please assign this issue to me
Huh. I was able to assign you. I thought folks had to be in the grpc org for that to work.
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));
}
My question is where should I create the java file for method toNanosSaturated(delay)
I thank you in advance for your patience:)
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)
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.
Cool. Thanks! I got two more questions @ejona86 @kluever
-
https://github.com/grpc/grpc-java/blob/4d3a29b2af2cd2b7b0a87d3702e298a8cfdcf238/api/src/main/java/io/grpc/SynchronizationContext.java#L177-L179 In the above method there is requirement for
initialDelay. How should I obtain it fromjava.time.Duration? -
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?
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);
}
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!
TimeUnit.NANOSECONDS.convert(duration)looks better thanDuration.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.
Two slight corrections to the above comment:
duration.toNanos()throws an exception if the duration is greater than 2^63 -1 nanosecondsTimeUnitconversion APIs won't overflow either, they silently saturate toLong.MAX_VALUE
About the 2nd point, yes, by overflow I meant the overflow and its handling in the exception block that we call saturation.
@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;
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.
@ejona86 can you also look at the concern I raised above about using the TimeUnit.NANOSECONDS.convert(long, TimeUnit) approach?
We used to use
-sourceand-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.
@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().
@ejona86
I've done the necessary changes and raised PR, kindly confirm.
https://github.com/grpc/grpc-java/pull/11562