Skip to content

Conversation

ovv
Copy link
Contributor

@ovv ovv commented Jun 20, 2025

A configuration with valid_from=None is always valid

@ovv ovv marked this pull request as draft June 23, 2025 08:57
@ovv ovv force-pushed the ovv/lookup-quix-config-service-valid-from branch from 7179514 to 5cc2844 Compare June 26, 2025 12:52
@ovv ovv requested a review from Copilot June 26, 2025 15:16
Copilot

This comment was marked as outdated.

@ovv ovv force-pushed the ovv/lookup-quix-config-service-valid-from branch from c7129e5 to fff3b4f Compare July 1, 2025 09:54
@ovv ovv requested a review from Copilot July 1, 2025 09:54
Copilot

This comment was marked as outdated.

ovv added 2 commits July 1, 2025 12:50
…ive tests

- Refactor Configuration.find_valid_version and _find_versions to handle edge cases
- Add tests the quix configuration service lookup
@ovv ovv force-pushed the ovv/lookup-quix-config-service-valid-from branch from fff3b4f to 24bedb6 Compare July 1, 2025 10:50
@ovv ovv marked this pull request as ready for review July 1, 2025 11:09
@ovv ovv requested a review from Copilot July 10, 2025 14:43
Copy link
Contributor

@Copilot Copilot AI left a 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 be None and maps it to 0 ms in ConfigurationVersion.from_event.
  • find_valid_version and _find_versions have been refined to correctly cache and select versions, including those with valid_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 a None valid_from is treated as always valid (initialized to 0 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 with valid_from equal to 0 (i.e., no timestamp) are considered valid from the very beginning.
    def find_valid_version(self, timestamp: int) -> Optional[ConfigurationVersion]:

gwaramadze
gwaramadze previously approved these changes Jul 15, 2025
Copy link
Contributor

@gwaramadze gwaramadze left a 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.

…/models.py

Co-authored-by: Remy Gwaramadze <gwaramadze@users.noreply.github.com>
ovv and others added 3 commits July 15, 2025 17:32
…/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>
@ovv ovv merged commit 6eb714a into main Jul 16, 2025
4 checks passed
@daniil-quix daniil-quix deleted the ovv/lookup-quix-config-service-valid-from branch July 16, 2025 11:58
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