-
Notifications
You must be signed in to change notification settings - Fork 85
Handle valid_from=None in configuration version selection #946
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
Conversation
7179514
to
5cc2844
Compare
c7129e5
to
fff3b4f
Compare
…ive tests - Refactor Configuration.find_valid_version and _find_versions to handle edge cases - Add tests the quix configuration service lookup
fff3b4f
to
24bedb6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the handling of valid_from=None
by treating configurations without a timestamp as always valid, and updates the version selection logic and tests accordingly.
- Event metadata now allows
valid_from
to beNone
and maps it to0
ms inConfigurationVersion.from_event
. find_valid_version
and_find_versions
have been refined to correctly cache and select versions, including those withvalid_from=0
.- A comprehensive test suite was added to cover
None
timestamps and edge cases in version lookup and join operations.
Comments suppressed due to low confidence (2)
quixstreams/dataframe/joins/lookups/quix_configuration_service/models.py:157
- [nitpick] Consider updating the docstring for
ConfigurationVersion.from_event
to clarify that aNone
valid_from
is treated as always valid (initialized to0
milliseconds).
raw_valid_from = event["metadata"]["valid_from"]
quixstreams/dataframe/joins/lookups/quix_configuration_service/models.py:241
- [nitpick] The docstring for
find_valid_version
could be extended to mention that versions withvalid_from
equal to0
(i.e., no timestamp) are considered valid from the very beginning.
def find_valid_version(self, timestamp: int) -> Optional[ConfigurationVersion]:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only a couple of minor comments. Ping me if you decide to accept any.
quixstreams/dataframe/joins/lookups/quix_configuration_service/models.py
Outdated
Show resolved
Hide resolved
quixstreams/dataframe/joins/lookups/quix_configuration_service/models.py
Outdated
Show resolved
Hide resolved
tests/test_quixstreams/test_dataframe/test_joins/test_lookup_quix_config.py
Outdated
Show resolved
Hide resolved
tests/test_quixstreams/test_dataframe/test_joins/test_lookup_quix_config.py
Show resolved
Hide resolved
tests/test_quixstreams/test_dataframe/test_joins/test_lookup_quix_config.py
Show resolved
Hide resolved
…/models.py Co-authored-by: Remy Gwaramadze <gwaramadze@users.noreply.github.com>
…/models.py Co-authored-by: Remy Gwaramadze <gwaramadze@users.noreply.github.com>
…uix_config.py Co-authored-by: Remy Gwaramadze <gwaramadze@users.noreply.github.com>
A configuration with
valid_from=None
is always valid