fix: timedelta to ms conversion and timedelta to deadline conversion #442
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Because a common concern in the library is expressing a `timedelta` object in milliseconds, we add a utility method to do that in one place. We also add unit tests to exhaustively verify behavior.
Use utility function where appropriate. This fixes the grpc configuration conversion which was truncating milliseconds.
Previously we were truncating the deadline to be whole seconds. This was in error: the grpc timeout parameter accepts fractional seconds to account for milliseconds.
malandis
commented
Mar 4, 2024
| @@ -154,7 +155,7 @@ def __init__(self, configuration: Configuration, credential_provider: Credential | |||
| self._endpoint = endpoint | |||
|
|
|||
| default_deadline: timedelta = configuration.get_transport_strategy().get_grpc_configuration().get_deadline() | |||
| self._default_deadline_seconds = int(default_deadline.total_seconds()) | |||
| self._default_deadline_seconds = default_deadline.total_seconds() | |||
There was a problem hiding this comment.
grpc's timeout takes fractional seconds to account for milliseconds, and timedelta.total_seconds returns fractional seconds, so we needn't truncate
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR corrects two bugs.
First we add a utility function to convert timedelta objects to milliseconds, unit tests, and refactor code to use this across the board. Previously we were converting timedeltas to milliseconds in several places which led to bugs when calling
.seconds(not fractional) vstotal_seconds()(fractional). Calling the former was truncating the milliseconds portion.Second we correct a bug the gRPC timeout. After reading from the config, we converted the timedelta to total whole seconds. This again truncated the milliseconds portion; because the gRPC parameter accepts fractional seconds, we should not have been truncating.