Skip to content
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

Jump to bottom

Feat: Add thread-safe UserAttributes in read/write operations. #253

Merged
merged 60 commits into from
Dec 10, 2020
Merged

Feat: Add thread-safe UserAttributes in read/write operations. #253

merged 60 commits into from
Dec 10, 2020

Conversation

mnoman09
Copy link
Contributor

@mnoman09 mnoman09 commented Dec 8, 2020

Summary

  • Copying UserAttributes to make it thread safe.
  • Made All fields in OptimizelyUserContext private and added getter methods

Test plan

Updated all Unit tests

mnoman09 and others added 30 commits November 5, 2020 20:48
@mnoman09
Added OptimizelyUserContext and basic functionalities
01bad8f 
and classes related to it
@mnoman09
Added DecisionTests
35c5e76
@mnoman09
Added classes of optimizelyDecision in net35 file
f79974c
@mnoman09
Updated decisionmessage class and added decisionMessage test
7531f3c
@mnoman09
Added OptimizelyDecisionTest
f006b08
@mnoman09
Added CreateUserContext in Optimizely
ac0c6a5
@mnoman09
Added UserContext add reason logs
6c9ac4d 
BugFix of userAttributes
Unit tests fix
@mnoman09
Revert "Added UserContext add reason logs"
9e36946 
This reverts commit 6c9ac4d.
@mnoman09
Added UserContext add reason logs
8c78330 
BugFix of userAttributes
Unit tests fix
@mnoman09
Added TrackEvent, DecideForKeys and DecideAllFunction
ee465c3 
Added Unit tests of above functions
@mnoman09
Replaced list of reasons to array.
591c770 
Throwing not implemented exceptions for not implemented functions in optimizelyUserContext
Added detailed documentation of optimizelyUserContext and Decision class
@mnoman09
Merge branch 'mnoman/user-context' into mnoman/decideapi
f644b24 
# Conflicts:
#	OptimizelySDK.Tests/OptimizelyUserContextTest.cs
#	OptimizelySDK/OptimizelyUserContext.cs
@mnoman09
Merge remote-tracking branch 'origin/master' into mnoman/user-context
ad77477
@mnoman09
Merge branch 'mnoman/user-context' into mnoman/decideapi
bab871e 
# Conflicts:
#	OptimizelySDK/Optimizely.cs
@mnoman09
Merge branch 'mnoman/decideapi' into mnoman/decideAllApi
2c0c3ec 
# Conflicts:
#	OptimizelySDK/OptimizelyUserContext.cs
@mnoman09
Bug fix
c3db077
@mnoman09
Resolved comments of adding null check before reasons
055bb93
@mnoman09
removed underscores
5ce1419
@mnoman09
Merge branch 'mnoman/user-context' into mnoman/decideapi
1e388b7
@mnoman09
Merge branch 'mnoman/decideapi' into mnoman/decideAllApi
a7e684b
@mnoman09
Added Decide Options unit tests
a424fef
@mnoman09
Added track event tests and fixes defaultDecideOptions
7eeb588
@mnoman09
Changed UserAttribute to Attributes
8e1750e
@mnoman09
Merge branch 'master' into mnoman/user-context
1163cde
@mnoman09
Merge branch 'mnoman/user-context' into mnoman/decideapi
6330750
@mnoman09
Merge branch 'mnoman/decideapi' into mnoman/decideAllApi
9ec58d3
@mnoman09
Added DecisionNotificationType flag
980abd0
@mnoman09
Added check of Key != null in decide
14769f8
@mnoman09
Added Additional unit tests of optimizelyUserContext
fafb14c
@mnoman09
Merge branch 'mnoman/user-context' into mnoman/decideapi
f9d3939 
# Conflicts:
#	OptimizelySDK.Tests/OptimizelyUserContextTest.cs
@mnoman09
Merge branch 'master' into mnoman/decideapi
73cd357 
# Conflicts:
#	OptimizelySDK.Tests/OptimizelyDecisions/OptimizelyDecisionTest.cs
#	OptimizelySDK.Tests/OptimizelyUserContextTest.cs
#	OptimizelySDK/Optimizely.cs
#	OptimizelySDK/OptimizelyDecisions/DefaultDecisionReasons.cs
#	OptimizelySDK/OptimizelyUserContext.cs
@mnoman09
Merge branch 'mnoman/decideapi' into mnoman/decideAllApi
fb2ebe4
@mnoman09
Sending copy of userattributes to make it thread safe
d253a13
@mnoman09 mnoman09 requested a review from a team as a code owner December 8, 2020 14:33
@mnoman09 mnoman09 requested review from msohailhussain and removed request for a team December 8, 2020 14:40
jaeopt
jaeopt requested changes Dec 8, 2020
View reviewed changes
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

Missing a mutex lock while making a copy. Other than that, all looks good!

OptimizelySDK/OptimizelyUserContext.cs Outdated

public UserAttributes GetAttributes()
{
return new UserAttributes(Attributes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need a mutex protection for this copy with the same lock for setAttribute?

msohailhussain
msohailhussain reviewed Dec 8, 2020
View reviewed changes
OptimizelySDK/OptimizelyUserContext.cs Outdated

public UserAttributes GetAttributes()
{
return new UserAttributes(Attributes);
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, mutex needed.

@jaeopt jaeopt mentioned this pull request Dec 8, 2020
Merged
@msohailhussain msohailhussain requested a review from jaeopt December 8, 2020 19:27
msohailhussain
msohailhussain approved these changes Dec 8, 2020
View reviewed changes
Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

lgtm

jaeopt
jaeopt approved these changes Dec 8, 2020
View reviewed changes
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

LGTM

@mnoman09
replaced copiedOptions = copiedOptions.Concat(options).Concat(Default…
fa90e13 
…DecideOptions).ToArray();

with
                copiedOptions = options.Union(DefaultDecideOptions).ToArray();
@mnoman09
Merge remote-tracking branch 'origin/master' into mnoman/decideAllApi
0f25fb2 
# Conflicts:
#	OptimizelySDK.Tests/OptimizelyUserContextTest.cs
#	OptimizelySDK/Optimizely.cs
@mnoman09
Merge branch 'mnoman/decideAllApi' into mnoman/thread-safe-userattrib…
ecbd2a4 
…utes

# Conflicts:
#	OptimizelySDK/Optimizely.cs
@jaeopt jaeopt mentioned this pull request Dec 9, 2020
Closed
Base automatically changed from mnoman/decideAllApi to master December 9, 2020 20:03
msohailhussain and others added 3 commits December 9, 2020 12:13
@msohailhussain
temporarily added.
24405c3
@mnoman09
Merge remote-tracking branch 'origin/master' into mnoman/thread-safe-…
2585e5c 
…userattributes

# Conflicts:
#	OptimizelySDK.Tests/OptimizelyTest.cs
#	OptimizelySDK.Tests/OptimizelyUserContextTest.cs
#	OptimizelySDK/Optimizely.cs
#	OptimizelySDK/OptimizelyUserContext.cs
@mnoman09
Removed unused variable
19f109e
@jaeopt jaeopt merged commit 5754452 into master Dec 10, 2020
@jaeopt jaeopt deleted the mnoman/thread-safe-userattributes branch December 10, 2020 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msohailhussain msohailhussain msohailhussain approved these changes

@jaeopt jaeopt jaeopt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mnoman09 @msohailhussain @jaeopt