8000 feat: Added ODPManager implementation by mikechu-optimizely · Pull Request #322 · optimizely/csharp-sdk · GitHub
[go: up one dir, main page]
Skip to content

feat: Added ODPManager implementation #322

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.

Sign up for GitHub

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

Merged
merged 18 commits into from
Dec 7, 2022
Merged

feat: Added ODPManager implementation #322

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
dd028bf
Initial ODP Manager
mikechu-optimizely Nov 23, 2022
a317407
Outline unit tests
mikechu-optimizely Nov 23, 2022
443164c
WIP Unit tests + changes to CUD
mikechu-optimizely Nov 28, 2022
faea784
Implement fetch, identify, and send events with Builder pattern
mikechu-optimizely Nov 28, 2022
191a6ec
WIP Fixing final unit test
mikechu-optimizely Nov 28, 2022
f6276bc
Finish unit tests
mikechu-optimizely Nov 29, 2022
4f629fa
Add documentation
mikechu-optimizely Nov 29, 2022
bbf8ac1
Lint fixes
mikechu-optimizely Nov 29, 2022
4a54b4c
Pull request code revisions + additional unit tests
mikechu-optimizely Dec 1, 2022
0995694
Lint fixes
mikechu-optimizely Dec 1, 2022
88b6684
Change to use string array over List
mikechu-optimizely Dec 1, 2022
29514ff
Clean usings
mikechu-optimizely Dec 1, 2022
22b04ae
Add new OdpConfig test
mikechu-optimizely Dec 1, 2022
dcafcee
Small refactors
mikechu-optimizely Dec 1, 2022
8747556
Pull request code review changes
mikechu-optimizely Dec 2, 2022
dfaa27b
Code review changes
mikechu-optimizely Dec 6, 2022
c528dce
Mark _odpConfig as volatile
mikechu-optimizely Dec 6, 2022
a5b28e6
Return `volatile` denotation
mikechu-optimizely Dec 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Code review changes
  • Loading branch information
@mikechu-optimizely
mikechu-optimizely committed Dec 6, 2022
commit dfaa27b24faea51b2e901d8a68da1dbca135ea1f
50 changes: 8 additions & 42 deletions OptimizelySDK/Odp/OdpConfig.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,59 +25,25 @@ public class OdpConfig : IEquatable<OdpConfig>
/// <summary>
/// Public API key for the ODP account from which the audience segments will be fetched (optional).
/// </summary>
private volatile string _apiKey;

public string ApiKey
{
get
{
return _apiKey;
}
private set
{
_apiKey = value;
}
}
public string ApiKey { get; private set; }

/// <summary>
/// Host of ODP audience segments API.
/// </summary>
private volatile string _apiHost;

public string ApiHost
{
get
{
return _apiHost;
}
private set
{
_apiHost = value;
}
}
public string ApiHost { get; private set; }

/// <summary>
/// All ODP segments used in the current datafile (associated with apiHost/apiKey).
/// </summary>
private volatile List<string> _segmentsToCheck;

public List<string> SegmentsToCheck
{
get
{
return _segmentsToCheck;
}
public List<string> SegmentsToCheck { get; private set; }

private set
{
_segmentsToCheck = value;
}
}

public OdpConfig(string apiKey, string apiHost, List<string> segmentsToCheck)
public OdpConfig(string apiKey = null, string apiHost = null,
List<string> segmentsToCheck = null
)
{
ApiKey = apiKey;
ApiHost = apiHost;
ApiKey = apiKey ?? string.Empty;
ApiHost = apiHost ?? string.Empty;
SegmentsToCheck = segmentsToCheck ?? new List<string>(0);
}

Expand Down
14 changes: 2 additions & 12 deletions OptimizelySDK/Odp/OdpManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public class OdpManager : IOdpManager, IDisposable
/// <summary>
/// Configuration used to communicate with ODP
/// </summary>
private volatile OdpConfig _odpConfig;
private OdpConfig _odpConfig;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you remove "volitile" here? Multiple thread read/write it here too.


/// <summary>
/// Manager used to handle audience segment membership
Expand Down Expand Up @@ -69,11 +69,6 @@ public bool UpdateSettings(string apiKey, string apiHost, List<string> segmentsT

_odpConfig = newConfig;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When ODPConfig is updated, the OdpConfig object itself is replaced in ODPManager, while copying fields in ODPEventManager and ODPSegmentManager. Can we make it consistent in either way?


// flush old events using old odp publicKey (if exists) before updating odp key.
// NOTE: It should be rare but possible that odp public key is changed for the same datafile (sdkKey).
// - Try to send all old events with the previous public key.
// - If it fails to flush all the old events here (network error), remaining events will be discarded.
EventManager.Flush();
EventManager.UpdateSettings(_odpConfig);

SegmentManager.ResetCache();
Expand Down Expand Up @@ -218,12 +213,7 @@ public OdpManager Build(bool asEnabled = true)
{
_logger = _logger ?? new DefaultLogger();
_errorHandler = _errorHandler ?? new NoOpErrorHandler();

if (_odpConfig == null)
{
_logger.Log(LogLevel.ERROR, "ODP Config via WithOdpConfig() required.");
return null;
}
_odpConfig = _odpConfig ?? new OdpConfig();

var manager = new OdpManager
{
Expand Down
0