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

Use Instant (monotonic) time instead of SystemTime #422

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

mislavn
Copy link
Contributor

@mislavn mislavn commented Jan 30, 2024

This continues on the #367 pull request.

Tarpc should use relative time, especially since RPC serialization no longer uses SystemTime but Duration.

With this commit Tarpc uses Instant time and on the edges converts Instant time to SystemTime for the Opentelemetry span's.

Copy link

google-cla bot commented Jan 30, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@tikue
Copy link
Collaborator

tikue commented Jan 30, 2024

I like this, thanks! I'll review this week.

@mislavn
Copy link
Contributor Author

mislavn commented Jan 30, 2024

Thanks for the quick reply!

I just want to add a bit of context. We use Tarpc to communicate with an embedded device that doesn't have an internal clock. During runtime we update the clock and that may cause RPC timeouts. This PR eliminates the issue.

I haven't added any new tests because I couldn't figure out an easy way of mocking the SystemTime for testing.

@tikue
Copy link
Collaborator

tikue commented Jan 30, 2024

Sounds like a cool application! Out of curiosity, what are you building?

tarpc/src/util.rs Outdated Show resolved Hide resolved
tarpc/src/client.rs Outdated Show resolved Hide resolved
tarpc/src/server.rs Outdated Show resolved Hide resolved
@mislavn
Copy link
Contributor Author

mislavn commented Jan 31, 2024

Sounds like a cool application! Out of curiosity, what are you building?

We are building X-ray detectors. Tarpc with channels is used for communication between our "microservices" on the detector, while for the public API facing the PC we use Cap'n proto but we still use Tarpc for testing and internal tools.

@tikue
Copy link
Collaborator

tikue commented Jan 31, 2024

There are some failures — I think there are just some missing imports for the TimeUntil trait.
image

@mislavn mislavn force-pushed the mn/monotonic_clock branch 2 times, most recently from c3aa688 to 11e8094 Compare January 31, 2024 21:42
This continues on the google#367 pull request.

Tarpc should use relative time, especially since RPC serialization no
longer uses SystemTime but Duration.

With this commit Tarpc uses Instant time and on the edges converts
Instant time to SystemTime for the Opentelemetry span's.

Signed-off-by: Mislav Novakovic <mislav.novakovic@gmail.com>
@mislavn
Copy link
Contributor Author

mislavn commented Jan 31, 2024

It should be OK now, I verified it locally.

@tikue tikue merged commit 9ed8696 into google:master Feb 1, 2024
5 checks passed
@tikue
Copy link
Collaborator

tikue commented Feb 1, 2024

Thanks again, I think this is a great change!

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 this pull request may close these issues.

2 participants