-
Notifications
You must be signed in to change notification settings - Fork 11
chore: Connect FDv2 data system. #108
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
…dd-fdv2-data-source-interfaces
…unchdarkly/java-core into rlamb/add-fdv2-data-source-interfaces
…ming-synchronizer
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.
This file is a move because of package boundaries. Not sure how we want them to shake out.
The HttpConfig -> HttpProperties I think was the missing component, and moving that around created further complications.
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.
This was a mistake in the implementation. So both polling and streaming have this update.
| } | ||
| } | ||
|
|
||
| private void runInitializers() { |
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.
I moved this for better logical organization.
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.
Still lots of work to do here. Outside the scope of this PR.
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.
Do you intend to add tests in the subsequent PR?
| FDv2SourceResult result = initializer.run().get(); | ||
| switch (result.getResultType()) { | ||
| case CHANGE_SET: | ||
| dataSourceUpdates.apply(result.getChangeSet()); |
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.
Actual change to the method. Apply the data.
59273f2 to
47e30b9
Compare
| this.readOnlyStore = new ReadonlyStoreFacade(store); | ||
| } | ||
|
|
||
| private static class FactoryWrapper<TDataSource> implements FDv2DataSource.DataSourceFactory<TDataSource> { |
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.
The FDv2 Data Source doesn't know anything about the dependencies. So this provides a parameter-less build method.
|
bugbot review |
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| DataStoreUpdatesImpl dataStoreUpdates = new DataStoreUpdatesImpl( | ||
| EventBroadcasterImpl.forDataStoreStatus(clientContext.sharedExecutor, logger)); | ||
|
|
||
| InMemoryDataStore store = new InMemoryDataStore(); |
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.
Persistent store configuration silently ignored in FDv2DataSystem
Medium Severity
The FDv2DataSystem.create() method always creates an InMemoryDataStore and never uses dataSystemConfiguration.getPersistentStore() or dataSystemConfiguration.getPersistentDataStoreMode(). Users who configure daemon mode or persistent store mode via DataSystemModes.daemon() or DataSystemModes.persistentStore() will find their persistent store configuration silently ignored, with data stored only in memory instead of the configured persistent store.
...dk/server/src/main/java/com/launchdarkly/sdk/server/subsystems/DataSourceBuilderContext.java
Outdated
Show resolved
Hide resolved
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.
Do you intend to add tests in the subsequent PR?
Yes, I am going to start working on making the implementation feature complete and tests. |
Note
Integrates the FDv2 data system end-to-end and replaces generic configurers with specific builders.
DataSystemComponentsexposes builders forpollingInitializer,pollingSynchronizer, andstreamingSynchronizer; retains FDv1 polling fallbackFDv2DataSystemto construct store, broadcasters,DataSourceUpdatesImpl, and build/run FDv2 initializers/synchronizers viaDataSourceBuildInputsFDv2DataSourceto useDataSourceUpdateSinkV2, applyCHANGE_SETs, and manage initializer/synchronizer lifecyclesSelectorSourceFacadeto fetch selectors from the transactional storeDefaultFDv2RequestorandStreamingSynchronizerImplnow send selector viabasisquery param (replacing version/state)DataSourceBuilderandDataSourceBuildInputs; updatedDataSystemBuilder,DataSystemConfiguration, andDataSystemModesto use typed builders; removed oldintegrations.DataSystemComponentsbasissemanticsWritten by Cursor Bugbot for commit 83b4217. This will update automatically on new commits. Configure here.