Skip to content

Conversation

@mjc1283
Copy link
Contributor

@mjc1283 mjc1283 commented Nov 7, 2019

The built-in default implementation of ExperimentOverrideStore should be production ready, so it should be safe to use concurrently.

In this PR:

  • The default implementation is renamed to MapExperimentOverridesStore
  • A sync.RWLock is added to MapExperimentOverridesStore
  • A SetVariation method is added for setting an override, which uses the mutex
  • The GetVariation method is updated to use the mutex
  • A new test is added that calls GetVariation and SetVariation concurrently from several goroutines

Copy link
Contributor

@thomaszurkan-optimizely thomaszurkan-optimizely left a comment

Choose a reason for hiding this comment

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

https://godoc.org/golang.org/x/sync/syncmap why not use sync map instead?

@mjc1283
Copy link
Contributor Author

mjc1283 commented Nov 8, 2019

@thomaszurkan-optimizely Based on reading this: https://golang.org/pkg/sync/#Map I gather that sync.Map, gives you better performance for certain use cases, and you don't have to implement locking yourself, but you give up type safety. Implementing locking is easy, and I don't know that we will always match one of those use cases, so it didn't seem worth giving up type safety. Let me know if you agree.

Also, aside from all that, can we even use it if we want our SDK to be usable in version <1.9?

Copy link
Contributor

@mikeproeng37 mikeproeng37 left a comment

Choose a reason for hiding this comment

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

Lgtm, just have a suggestion in the test

user3Decision, _ := s.overrideService.GetDecision(testDecisionContext, entities.UserContext{
ID: "test_user_3",
})
s.NotNil(user1Decision.Variation)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also assert that the decision variation key matches what you expected?

@mjc1283 mjc1283 merged commit fadd0ec into master Nov 8, 2019
@mjc1283 mjc1283 deleted the mcarroll/experiment-overrides-concurrent branch November 8, 2019 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants