Skip to content

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

Merged
kannanjgithub merged 37 commits intogrpc:masterfrom
SreeramdasLavanya:FixIssue10245
Oct 30, 2024
Merged

api: Add java.time.Duration overloads to CallOptions, AbstractStub#11562
kannanjgithub merged 37 commits intogrpc:masterfrom
SreeramdasLavanya:FixIssue10245

Conversation

@SreeramdasLavanya
Copy link
Copy Markdown
Contributor

@SreeramdasLavanya SreeramdasLavanya commented Sep 26, 2024

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

Fixes #10245

@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla bot commented Sep 26, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

}

public static Deadline after(Duration duration, Ticker ticker) {
return after(TimeUnit.NANOSECONDS.convert(duration.getSeconds(), TimeUnit.SECONDS),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@SreeramdasLavanya 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

import java.time.Duration;

@Internal
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this is public in a public package, so the @Internal is needed.

@grpc-kokoro grpc-kokoro removed 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
@SreeramdasLavanya 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
Copy link
Copy Markdown
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need @ExperimentalApi on all the new public API methods (those receiving Duration).


import java.time.Duration;

@Internal
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this is public in a public package, so the @Internal is needed.

…ving Duration and reverted LoadBalancer code.
@ejona86 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 25, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Oct 25, 2024
@ejona86 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 29, 2024
@grpc-kokoro 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 29, 2024
return new ScheduledHandle(runnable, future);
}

@ExperimentalApi("https://github.com/grpc/grpc-java/issues/10245")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These links should point to a new issue created to track stabilizing this API. (See issues with the "experimental API" label for examples.)

@kannanjgithub kannanjgithub added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Oct 30, 2024
@grpc-kokoro 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 30, 2024
@kannanjgithub kannanjgithub merged commit 766b923 into grpc:master Oct 30, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 29, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

4 participants