refactor: hide data source configuration behind dataSystem interface #189
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cwaldren-ld
force-pushed
the
cw/sdk-9-refactor-internal-data-interactions
branch
from
26d1cee
to
bd44019
Compare
cwaldren-ld
commented
Sep 16, 2024
| @@ -9,13 +9,13 @@ import ( | |||
| ) | |||
|
|
|||
| type dataStoreEvaluatorDataProviderImpl struct { | |||
There was a problem hiding this comment.
This change is because the existing read/write enabled DataStore interface isn't actually needed in the evaluator. It only needs to be able to retrieve segments/flags.
cwaldren-ld
commented
Sep 16, 2024
| @@ -24,21 +26,6 @@ type DataStore interface { | |||
| // data, and then delete any previously stored items that were not in the input data. | |||
There was a problem hiding this comment.
These were moved into ReadOnlyStore
cwaldren-ld
force-pushed
the
cw/sdk-9-refactor-internal-data-interactions
branch
from
bd44019
to
de69ac4
Compare
cwaldren-ld
force-pushed
the
cw/sdk-9-refactor-internal-data-interactions
branch
from
de69ac4
to
d09e32d
Compare
cwaldren-ld
commented
Sep 16, 2024
| @@ -583,13 +554,18 @@ func (client *LDClient) SecureModeHash(context ldcontext.Context) string { | |||
| // this does not guarantee that the flags are up to date; if you need to know its status in more detail, use | |||
| // [LDClient.GetDataSourceStatusProvider]. | |||
| // | |||
There was a problem hiding this comment.
Added this comment because that's what our unit tests expect. Didn't see it documented.
cwaldren-ld
commented
Sep 16, 2024
| // If this value is false, it means the client has not yet connected to LaunchDarkly, or has permanently | ||
| // failed. See [MakeClient] for the reasons that this could happen. In this state, feature flag evaluations | ||
| // will always return default values-- unless you are using a database integration and feature flags had | ||
| // already been stored in the database by a successfully connected SDK in the past. You can use | ||
| // [LDClient.GetDataSourceStatusProvider] to get information on errors, or to wait for a successful retry. | ||
| func (client *LDClient) Initialized() bool { | ||
| return client.dataSource.IsInitialized() | ||
| if client.offline { |
There was a problem hiding this comment.
This logic used to work because if the client was in offline mode, then a NullDataSource was instantiated (which returned true for IsInitialized()).
Since dataSystem doesn't expose IsInitialized - just the DataAvailability - we need to check that here explicitly. Simpler to understand.
keelerm84
approved these changes
Sep 17, 2024
cwaldren-ld
deleted the
cw/sdk-9-refactor-internal-data-interactions
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change defines a new
dataSysteminterface, used internally byLDClientto implement all functionality related to data store and data source interactions.The purpose is to be able to swap out the implementation with one suited for FDv2 in a future PR.