feat(dfm): Datafile management implementation #160
Conversation
…rojectConfig as its implementation. Created StaticProjectConfigManager and HttpProjectConfigManager for handling datafile retrievel. Created PollingProjectConfigManager for retrieving ProjectConfig on the specified interval. Added OnReady future in PollingProjectConfigManager class. Added unit tests.
# Conflicts: # OptimizelySDK/DatafileManagement/DatafileProjectConfig.cs
|
|
||
| namespace OptimizelySDK.DatafileManagement | ||
| { | ||
| public class AtomicProjectConfigManager : ProjectConfigManager |
There was a problem hiding this comment.
There isn't anything about this class that makes it atomic. If that is the case, can we change the name to something else or actually make it atomic?
| //t.Start(); | ||
| //return t; | ||
| } | ||
| static ProjectConfig ParseProjectConfig(string datafile) |
There was a problem hiding this comment.
I don't think this is the right place to put it. If you look at the Java SDK, the notion of a ProjectConfig backed by a Datafile is split from this and is defined at a higher level: https://github.com/optimizely/java-sdk/blob/master/core-api/src/main/java/com/optimizely/ab/config/DatafileProjectConfig.java
The idea is that we can have other types of Datafile-backed project config that don't necessarily have to be polled, such as push-based with websockets or server-sent events.
|
|
||
| namespace OptimizelySDK | ||
| { | ||
| public interface ProjectConfig |
There was a problem hiding this comment.
ProjectConfig can be more abstract than this. We can have project configs later on that are not based on datafile, but can be based on a different structure. Here is the abstraction approach we took in the Java SDK:
For this reason, also, we should change the name of the package to ProjectConfig instead of DatafileManagement so we don't tie it to just the Datafile transport type.
| /// set by the user by calling setForcedVariation (it is not the same as the | ||
| /// whitelisting forcedVariations data structure in the Experiments class). | ||
| /// </summary> | ||
| private Dictionary<string, Dictionary<string, string>> _ForcedVariationMap; |
There was a problem hiding this comment.
I'd suggest moving the forced variation state to the decision service, as we've done in other SDKs.
There was a problem hiding this comment.
It's been moved in sohail/optimizelydfm (Will add PR today)
There was a problem hiding this comment.
Mostly good. There are a few nits and we also need to move the forced variation logic into the decision service. A good way to do that would be to open up another PR against sohail/dfmcomplete with that change. We can review and merge and then rebase this PR for merging.
| @@ -0,0 +1,643 @@ | |||
| /* | |||
| * Copyright 2017-2019, Optimizely | |||
There was a problem hiding this comment.
nit: Should just be 2019
| /// set by the user by calling setForcedVariation (it is not the same as the | ||
| /// whitelisting forcedVariations data structure in the Experiments class). | ||
| /// </summary> | ||
| private Dictionary<string, Dictionary<string, string>> _ForcedVariationMap; |
| private Dictionary<string, List<string>> ExperimentFeatureMap = new Dictionary<string, List<string>>(); | ||
|
|
||
|
|
||
| //========================= Callbacks =========================== |
There was a problem hiding this comment.
nit: These aren't really callbacks are they?
|
|
||
| namespace OptimizelySDK.Config | ||
| { | ||
| public class FallbackProjectConfigManager : ProjectConfigManager |
There was a problem hiding this comment.
Is there gonna be more logic for the "fallback"? I think you mentioned you'd have that in a separate PR. In that case just add a //TODO here for it
There was a problem hiding this comment.
nope, no more logic, in other PR, I will remov SetConfig, and ProjectConfig will only be set by constructor.
| public Task OnReady() | ||
| { | ||
| return CompletableConfigManager.Task; | ||
| //Task t = new Task(() => { |
I removed in this PR. |
msohailhussain
changed the title
Summary
All details are in the doc.
Test plan
Issues