feat: data system config builders [3] #292
Conversation
cwaldren-ld
force-pushed
the
cw/initial-datasystem-configs
branch
from
01c0c13 to
e17ee1b
Compare
cwaldren-ld
force-pushed
the
cw/initial-datasystem-configs
branch
from
e17ee1b to
c5a8d3d
Compare
| @@ -0,0 +1,27 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
This file pulls in all the shared builders, making the templated versions available within the server_side::config::builders namespace. At the same time it pulls in the locally defined builders, so they all exist at the same namespace level.
| @@ -0,0 +1,18 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
This is mostly a placeholder and not implemented yet.
| @@ -0,0 +1,16 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
Mostly a placeholder, not built yet.
| using BackgroundSync = BackgroundSyncBuilder; | ||
| using LazyLoad = LazyLoadBuilder; | ||
|
|
||
| DataSystemBuilder& Disabled(bool disabled); |
There was a problem hiding this comment.
Right now the status quo is to have an offline boolean that disables the data source + events. I was thinking it'd be nice to have a clearly labeled Disabled() call for just the sources:
auto cfg = ConfigBuilder("sdk123")
.DataSystem().Disabled(true)That way we can say "Offline mode is shorthand for calling Events().Disabled() and DataSystem.Disabled()" or similar.
| * which leaves stale items in the cache until they can be refreshed. \param | ||
| * policy The EvictionPolicy. \return Reference to this. | ||
| */ | ||
| LazyLoadBuilder& CacheEviction(EvictionPolicy policy); |
There was a problem hiding this comment.
Ryan's existing work also had the Cache eviction interval and a mode for active eviction. We can have this, but I haven't added it here (though I suspect we could add it later just as well.)
| @@ -0,0 +1,8 @@ | |||
| #pragma once | |||
|
|
|||
| @@ -0,0 +1,9 @@ | |||
| #pragma once | |||
|
|
||
| EvictionPolicy eviction_policy; | ||
| std::chrono::milliseconds eviction_ttl; | ||
| std::shared_ptr<server_side::data_interfaces::ISerializedDataPullSource> |
There was a problem hiding this comment.
This is the interface we'd expect users to implement if they have a custom persistent store (ISerializedDataPullSource).
The shared_ptrto interface approach is pretty general - we could have a higher level constructor to make a Redis client object, or let people pass in their own.
We could also have source be a variant<shared_ptr<ISerializedDataPullSource>, SomeRedisArgs> instead if we wanted to defer construction to SDK creation time and totally encapsulate it.
But, I think the current way is nice because then the Client itself has zero knowledge that it's dealing with Redis. Seems more sustainable.
| @@ -0,0 +1,10 @@ | |||
| #pragma once | |||
| return *this; | ||
| } | ||
|
|
||
| tl::expected<built::DataSystemConfig, Error> DataSystemBuilder::Build() const { |
There was a problem hiding this comment.
This Build() here is simple in concept but just gross to look at.
The problem is unifying the return values of LazyLoad::Build (fallible, tl::expected) and BackgroundSync::Build (infallible, since if no source is provided we default to streaming.)
Instead of doing it in build, could instead call Build in the ::Method implementations. Then we'd need to store an:
std::variant<
tl::expected<LazyLoadConfig, Error>,
tl::expected<BackgroundSyncConfig, Error>
>member variable.
Or ideally a tl::expected<std::variant<LazyLoadConfig, BackgroundSyncConfig>, Error>. Maybe there's a tl::expected magic method for that.
Will be looking at this in future PRs.
| @@ -40,7 +34,7 @@ tl::expected<Config, Error> ConfigBuilder::Build() const { | |||
| if (sdk_key.empty()) { | |||
| return tl::make_unexpected(Error::kConfig_SDKKey_Empty); | |||
| } | |||
| auto offline = offline_.value_or(Defaults::Offline()); | |||
There was a problem hiding this comment.
Ignore this. Had some back-and-forth thoughts about offline mode; will add it back in as a top-level config since it's convenient / status quo.
cwaldren-ld
changed the title
| @@ -2,54 +2,50 @@ | |||
|
|
|||
| namespace launchdarkly::server_side { | |||
|
|
|||
| using namespace launchdarkly::config::shared; | |||
|
|
|||
| Config::Config(std::string sdk_key, | |||
There was a problem hiding this comment.
Removing redundant namespace qualifiers + plumbing the new DataSystemConfig
This introduces a set of configuration objects & builders for the server-side Data System.
These are defined in
libs/server-sdkrather than in common, since they don't apply to the client at the moment. In a future client major version, we could attempt to unify them.