-
Notifications
You must be signed in to change notification settings - Fork 45
feat: adding file data source as an intializer #381
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
This commit will make it so that the client will only report initalized when a valid selector is present in the basis
570b033 to
064f65c
Compare
| assert flag is not None | ||
|
|
||
|
|
||
| def test_fdv2_with_file_to_polling_initializers(): |
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.
These tests reference polling, but they are using the test data sources instead. So we should probably update the language.
| assert set_on_ready.wait(1), "Data system did not become ready in time" | ||
| assert count == 2, "Invalid initializer process" |
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.
Are we sure there isn't a timing issue in this test? You are testing for 2 callbacks, but it may actually do 3 -- one for each initialization, and then another when the sync runs the first time.
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.
Ah I think I misunderstood these test datasource... I thought that the test datasources will only give a response/trigger a callback if we call the update function? eg https://github.com/launchdarkly/python-server-sdk/pull/381/files/064f65c76135c385a6fdcb0209caa0e7a8cf872c#diff-1cee15fc62f5439ab90479c81c5cf48672134012d9b81254303605674e9c4fb4R467 ...if not then what would be a good way to test this? just wait for the synchronizer to finish and make it count 3?
UPDATE: yea I played around a bit more and there is a race condition and I think these tests pass because I immediately close the client after the ready event. Will change.
| fdv2.flag_tracker.add_listener(listener) | ||
|
|
||
| fdv2.start(set_on_ready) | ||
| assert set_on_ready.wait(1), "Data system did not become ready in time" |
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.
Same question here. Is it possible the change handler could run twice?
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.
So assuming that we don't count the synchronizer, the change handler should only run once as the file data source will be skipped in this instance so if the change handler runs more than once during the initialization phase then it is a problem.
- modified the tests to be more robust - modified wording on test to be more clear on what is being tested - removed unnecessary check for ready event
|
@keelerm84 I changed up the tests a bit and I think they shouldn't run into race conditions anymore |
Requirements
This PR will:
Note
Introduces a file-based initializer and updates FDv2 to mark ready only when an initializer provides a selector, with tests validating initializer ordering and early completion.
file_ds_builder(paths)returning_FileDataSourceV2initializer inldclient/datasystem.py; import_FileDataSourceV2.ldclient/impl/datasystem/fdv2.py):basis.change_set.selectoris defined; then return to proceed to synchronizers.ldclient/testing/impl/datasystem/test_fdv2_datasystem.py):file_ds_builderwith temp JSON files.Written by Cursor Bugbot for commit 04a2c53. This will update automatically on new commits. Configure here.