-
Notifications
You must be signed in to change notification settings - Fork 31
feat: add FDv2 FileDataSource initializer #1010
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
base: main
Are you sure you want to change the base?
Conversation
this commit will introduce the implementation of FDv2 File initiator
|
@launchdarkly/browser size report |
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
packages/shared/sdk-server/src/data_sources/fileDataInitilizerFDv2.ts
Outdated
Show resolved
Hide resolved
5d8cadd to
cf28ade
Compare
| | DeleteObject | ||
| | PayloadTransferred | ||
| | GoodbyeObject | ||
| | ErrorObject; |
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.
If new values are added to the protocol, is the code well behaved? Same for nearby types.
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.
yea payload changes will potentially break this, but that is probably true even if we didn't declare these types?
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.
Double check logic in the processFDv1FlagsAndSegments in PollingProcessorFDv2 matches this and/or consider commonizing.
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 yea the logic is the same! Let me think of a way to merge the logic.
Now that I am thinking about this more, I don't think sdk-common is the right place to create this builder? I am not sure how FDv2 is going to show up for client side implementations so probably move this out to sdk-server then, if we find out that client implementation is similar, move it back out to sdk-common? @abarker-launchdarkly @kinyoklion thoughts?
Otherwise, I think the plan will be change this to a FDv1PayloadAdaptor instead of a builder.
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.
If the data inside the objects is opaque, then the the code itself would likely be the same for both of them.
I am fine with it not being common until we need it though.
A note specifically for internal/common code, but code we intend to use client-side we likely want to avoid classes and do this with functions, as generally speaking they end up smaller when minified. (Or old school function that returns object).
packages/shared/sdk-server/src/data_sources/fileDataInitilizerFDv2.ts
Outdated
Show resolved
Hide resolved
| 'DataSourceOptions', | ||
| typeof options.dataSource, | ||
| ), | ||
| ]; |
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.
Should a customer be able to configure to use only a file data source? @kinyoklion thoughts?
I think they should be able to configure just the file data source with polling and streaming disabled, but you'll have to confirm with others on team as I haven't dealt with file data source implementations.
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 think we discussed sync, but yeah it should be possible. This would be a typical use case with FDv1. Often times people use file data sources for testing purposes.
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.
Okay I think we can have a separate story to handle file synchronizer? I think it would be a fairly trivial implementation on top of this PR.
packages/shared/sdk-server/src/data_sources/fileDataInitilizerFDv2.ts
Outdated
Show resolved
Hide resolved
cf28ade to
15d65d7
Compare
24bee72 to
10363a9
Compare
packages/shared/sdk-server/src/data_sources/fileDataInitilizerFDv2.ts
Outdated
Show resolved
Hide resolved
this commit adds a fdv1 payload adaptor to process fdv1 data sets
b1b1883 to
e1b1349
Compare
this enables users to specify file data source initializer
e1b1349 to
718b11a
Compare
| dataCallback(false, { initMetadata, payload }); | ||
| }); | ||
|
|
||
| statusCallback(subsystemCommon.DataSourceState.Valid); |
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.
@tanderson-ld what does marking the state as valid do exactly? Is this what is telling the client that this initializer is successful?
This PR will introduce file data source initializer for FDv2 that could be specified by adding in an optional
intializerOptionto the FDv2 datasource option. If this option is not specified then the behavior should not change from before this change.Additionally:
customdata source options mode that allows users to specify the ordering of their initializers and synchronizersNOTE: will need to look into a problem where while the initializers are running in expected order, the client initialized event seems to be prematurely firing. Example with the following config:
STDOUT with the
sample-featureflag set tofalsein LD:I think this issue could be addressed in a separate change.
Note
Introduce FDv2 file-based initializer and customizable initializer/synchronizer pipeline, plus an adaptor to convert FDv1 payloads to FDv2 and related typing updates.
FDv1PayloadAdaptorto build FDv2 change sets from FDv1 payloads; export viainternal/fdv2/index.prototypes (EventType,ObjectKind, etc.); allow emptystateinpayload-transferredand optionalid.FileDataInitializerFDv2to load flags/segments from JSON or YAML (optional parser) and emit FDv2 payloads.FDv1PayloadAdaptorto handle FDv1 responses (selectorFDv1Fallback).dataSourceOptionsType: 'custom'with orderedinitializers(file/polling) andsynchronizers(streaming/polling); wire intoLDClientImpl.DEFAULT_STREAM_RECONNECT_DELAY; validation accepts custom options.FDv1PayloadAdaptor,FileDataInitializerFDv2, andCompositeDataSourcemulti-initializer behavior.Written by Cursor Bugbot for commit 718b11a. This will update automatically on new commits. Configure here.