api: Add java.time.Duration overloads to CallOptions, AbstractStub #11562

SreeramdasLavanya · 2024-09-26T13:33:07Z

Overloading API's containing long, TimeUnit with java.time.Duration

Fixes #10245

…h Duration" This reverts commit ab97045.

linux-foundation-easycla · 2024-09-26T13:33:13Z

The committers listed above are authorized under a signed CLA.

✅ login: SreeramdasLavanya (62a88ec, bd5a80a, ab97045, 6a713ed, db6c21f, 328bcbf, 4113845, c1090b7, 84a37e9, 6c56aae, 6a66054, 778cfb4, fef4c92, 4531bc2, 09c3509, e76d673, b905dfa, eb66fe5, 7207da8, 4d0aa82, f01209b, bf6c521, e64f659, af01908, 0b6c594, de2e185)

kannanjgithub · 2024-09-27T06:18:40Z

api/src/context/java/io/grpc/Deadline.java

@@ -95,6 +101,11 @@ public static Deadline after(long duration, TimeUnit units, Ticker ticker) {
    return new Deadline(ticker, units.toNanos(duration), true);
  }

+  public static Deadline after(Duration duration, Ticker ticker) {
+    return after(TimeUnit.NANOSECONDS.convert(duration.getSeconds(), TimeUnit.SECONDS),


I have a concern about the loss of precision, I have raised a question on Eric's comment to use TimeUnit.NANOSECONDS.convert(duration) instead of Duration.toNano().

As Eric replied, you should be using https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/concurrent/TimeUnit.html#convert(java.time.Duration) and not have to convert using the seconds value.

…adlineTest files

kannanjgithub · 2024-10-09T05:25:10Z

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

@@ -149,16 +151,16 @@ public final ScheduledHandle schedule(
      final Runnable task, long delay, TimeUnit unit, ScheduledExecutorService timerService) {
    final ManagedRunnable runnable = new ManagedRunnable(task);
    ScheduledFuture<?> future = timerService.schedule(new Runnable() {
-        @Override


Don't format lines unrelated to your changes, it will make git blame difficult to identify changes specific to a fix.

The formatting changes have not been reverted, here and in other places.

kannanjgithub · 2024-10-09T05:28:32Z

api/src/test/java/io/grpc/SynchronizationContextTest.java

@@ -72,7 +73,7 @@ public void uncaughtException(Thread t, Throwable e) {

  @Mock
  private Runnable task3;
-


Remove formatting changes in all unrelated places.

Can you take care of this comment? @SreeramdasLavanya

Whitespace formatting changes are still there in this fie.

stub/src/main/java/io/grpc/stub/AbstractStub.java

kannanjgithub · 2024-10-16T06:20:57Z

api/src/main/java/io/grpc/InternalTimeUtils.java

+
+@Internal
+public final class InternalTimeUtils {
+  public static long convert(Duration duration) {


Code coverage is not detecting that this method is used from tests for some reason. I don't know if it is unable to detect static import of a function. Can you try removing the static import of InternalTimeUtils.convert in the test class, and import the class only, and use InternalTimeUtils.convert( in the test code and see if the Codecov warning goes away.

kannanjgithub · 2024-10-16T06:20:58Z

api/src/main/java/io/grpc/InternalTimeUtils.java

+
+import java.time.Duration;
+
+@Internal


@internal is used to mark certain classes as should not be used by user code. This class being just a util helper, there is no need to mark it as Internal. Also drop the Internal prefix from the class and file name and make it just TimeUtils.

…c import for InternalTimeUtils.convert method

…d file changes

ejona86

We use commit prefixes without the "grpc-", so just "api: Changing...". Note that this is not "changing" to Duration. It is adding Duration.

ejona86 · 2024-10-17T14:18:15Z

api/src/main/java/io/grpc/TimeUtils.java

+
+import java.time.Duration;
+
+public final class TimeUtils {


Don't create a new public API. This should be package-private.

We are using it from a different package io.grpc.stub.AbstractStub too. We'd have to make a copy of this class then. Is that what we want? @ejona86

We have a way to expose internal classes/methods from api:

The class/method itself is package-private

Create a new class for the class name that needs exposing, prefixing the class name with Internal and marking it @Internal

Add accessors in that InternalFoo class.

So here you'd create a public InternalTimeUtils class that has convert() on it. We do the dance because 1) we hide Internal* classes from the javadoc, and 2) when we compile with Blaze we build the Internal* classes as a separate target which is visibility-restricted.

We normally do that for APIs needed to coordinate though, not utilities (core has most utilities). In this case, just duplicating the utility seems reasonable. We'll have at most three copies of it: api, stub, core. Most other things depend on core and it it can be public in core. Not the greatest, but it is small.

ejona86 · 2024-10-17T14:18:32Z

api/src/main/java/io/grpc/TimeUtils.java

+import java.time.Duration;
+
+public final class TimeUtils {
+  public static long convert(Duration duration) {


It is not at all clear you get nanos back. convertToNanos()? toNanos()?

kannanjgithub · 2024-10-21T05:22:48Z

api/src/main/java/io/grpc/TimeUtils.java

@@ -19,7 +19,7 @@
 import java.time.Duration;

 public final class TimeUtils {
-  public static long convert(Duration duration) {
+  public static long convertToNanos(Duration duration) {
    try {


@ejona86 Why is Codecov complaining even though the class is in fact covered by unit tests?

I don't see it complaining here. I see it complaining on the class. That is probably for the default public constructor. We should give this class a private, never-called constructor, FWIW. I think Codecov would still point out it isn't covered though.

SreeramdasLavanya and others added 9 commits September 20, 2024 14:25

SynchronizationContextTest changes for scheduleFixedDelay with Duration

ab97045

Revert "SynchronizationContextTest changes for scheduleFixedDelay wit…

fef4c92

…h Duration" This reverts commit ab97045.

SynchronizationContextTest changes for scheduleFixedDelay with Duration

6a713ed

SynchronizationContextTest checkstyle issues updated for issue10245

db6c21f

Merge branch 'grpc:master' into master

778cfb4

Merge branch 'grpc:master' into master

328bcbf

Merge branch 'grpc:master' into master

6a66054

Issue 10245 fix for long, TimeUnit to java.time.Duration to API's

84a37e9

Issue 10245 - Fixed Checkstyle issues

c1090b7

Issue 10245 - Fixed Checkstyle issues

6c56aae

kannanjgithub reviewed Sep 27, 2024

View reviewed changes

SreeramdasLavanya and others added 2 commits October 1, 2024 12:13

Merge branch 'grpc:master' into master

4113845

Issue 10245 - Reverted java.time.Duration changes for Deadline and De…

4531bc2

…adlineTest files

SreeramdasLavanya changed the title ~~Fix for long, TimeUnit to java.time.Duration changes to API's~~ grpc-api: Changing long, TimeUnit to java.time.Duration Oct 4, 2024

SreeramdasLavanya and others added 2 commits October 8, 2024 09:36

Merge branch 'grpc:master' into master

09c3509

Issue 10245 - Implementation for Duration.toNanos() changes

bd5a80a

SreeramdasLavanya mentioned this pull request Oct 8, 2024

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

Open

kannanjgithub reviewed Oct 9, 2024

View reviewed changes

stub/src/main/java/io/grpc/stub/AbstractStub.java Show resolved Hide resolved

SreeramdasLavanya and others added 7 commits October 9, 2024 11:09

grpc-api/stub: Implementation for InternalTimeUtil under grpc-api module

e64f659

grpc-api/stub: Added JUnit for the InternalTimeUtils changes

0b6c594

Merge branch 'master' into FixIssue10245

4d0aa82

Merge branch 'grpc:master' into master

62a88ec

Merge branch 'master' into FixIssue10245

b905dfa

grpc-api/stub: Modified JUnit for the InternalTimeUtils changes

eb66fe5

grpc-api: Whitespace formatting for unrelated code

e76d673

kannanjgithub reviewed Oct 16, 2024

View reviewed changes

SreeramdasLavanya added 4 commits October 16, 2024 12:41

grpc-api: Whitespace formatting for unrelated code and Reverted stati…

f01209b

…c import for InternalTimeUtils.convert method

grpc-api: Reverted static import for InternalTimeUtils.convert method

de2e185

grpc-api/stub: Renamed InternalTimeUtils to TimeUtils and its effecte…

7207da8

…d file changes

grpc-api: Formatted whitespaces

bf6c521

kannanjgithub approved these changes Oct 17, 2024

View reviewed changes

kannanjgithub requested a review from ejona86 October 17, 2024 06:26

ejona86 reviewed Oct 17, 2024

View reviewed changes

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Oct 17, 2024

grpc-kokoro removed kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run labels Oct 17, 2024

SreeramdasLavanya changed the title ~~grpc-api: Changing long, TimeUnit to java.time.Duration~~ api: Add java.time.Duration overloads to CallOptions, AbstractStub Oct 18, 2024

grpc-api/stub: Renamed convert() method to convertToNanos()

af01908

kannanjgithub reviewed Oct 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: Add java.time.Duration overloads to CallOptions, AbstractStub #11562

api: Add java.time.Duration overloads to CallOptions, AbstractStub #11562

SreeramdasLavanya commented Sep 26, 2024 •

edited

Loading

linux-foundation-easycla bot commented Sep 26, 2024 •

edited

Loading

kannanjgithub Sep 27, 2024

kannanjgithub Oct 3, 2024

kannanjgithub Oct 9, 2024

kannanjgithub Oct 10, 2024

kannanjgithub Oct 9, 2024

kannanjgithub Oct 15, 2024

kannanjgithub Oct 16, 2024

kannanjgithub Oct 16, 2024

kannanjgithub Oct 16, 2024

ejona86 left a comment

ejona86 Oct 17, 2024

kannanjgithub Oct 21, 2024

ejona86 Oct 21, 2024

ejona86 Oct 17, 2024

kannanjgithub Oct 21, 2024

ejona86 Oct 21, 2024

		@@ -72,7 +73,7 @@ public void uncaughtException(Thread t, Throwable e) {

		@Mock
		private Runnable task3;

api: Add java.time.Duration overloads to CallOptions, AbstractStub #11562

Are you sure you want to change the base?

api: Add java.time.Duration overloads to CallOptions, AbstractStub #11562

Conversation

SreeramdasLavanya commented Sep 26, 2024 • edited Loading

linux-foundation-easycla bot commented Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SreeramdasLavanya commented Sep 26, 2024 •

edited

Loading

linux-foundation-easycla bot commented Sep 26, 2024 •

edited

Loading