fix(notification): Remote notification only #219
Conversation
msohailhussain
changed the title
pkg/config/polling_manager_test.go
Outdated
| configManager.SyncConfig(mockDatafile2) | ||
| optimizelyConfig = configManager.GetOptimizelyConfig() | ||
| assert.Equal(t, "43", optimizelyConfig.Revision) | ||
| //mockRequester.AssertExpectations(t) |
There was a problem hiding this comment.
@msohailhussain , not sure why I could not check for requester assert expectation. Can you take a look ?
There was a problem hiding this comment.
I don't know how it was working before. When SyncConfig is called with payload in the argument, it means no Get call should be asserted. I have removed explicit SyncConfig call and now dependent on Start scheduled call. Very first call is called immediately, so no major wait in this unit test.
msohailhussain
changed the title
| projectConfigUpdateNotification := notification.ProjectConfigUpdateNotification{ | ||
| Type: notification.ProjectConfigUpdate, | ||
| Revision: cm.projectConfig.GetRevision(), | ||
| Revision: projectConfig.GetRevision(), |
There was a problem hiding this comment.
FYI. race condition fix
| mockDatafile2 := []byte(`{"revision":"43"}`) | ||
| sdkKey := "test_sdk_key" | ||
|
|
||
| mockRequester := new(MockRequester) |
There was a problem hiding this comment.
This mockRequester isn't passed as an option to NewPollingProjectConfigManager via WithRequester, so the assertion on 316 isn't useful, right?
There was a problem hiding this comment.
Thanks @mjc1283 for pointing out, i have added requester there.
|
|
||
| // Send sends the notification to the registered handlers | ||
| func (am *AtomicManager) Send(notification interface{}) { | ||
| am.lock.Lock() |
There was a problem hiding this comment.
If the user calls Remove in their notification callback, does that cause deadlock?
There was a problem hiding this comment.
I can try a sample code, and can check.
|
One more question here is, if user calls |
There was a problem hiding this comment.
What this is making pretty clear is that the SyncConfig method is overloaded and causes confusion. We should have distinct methods for SetConfig(datafile []byte) and PollConfig(). I think this will simplify the logic and avoid the branching and checking that currently going on.
| configManager := NewPollingProjectConfigManager(sdkKey, WithRequester(mockRequester)) | ||
| eg.Go(configManager.Start) | ||
|
|
||
| m := sync.RWMutex{} |
There was a problem hiding this comment.
I'd be surprised if the ++ operator was not already atomic, but there is an atomic increment as part of the sync package https://gobyexample.com/atomic-counters that you should be able to use instead. Since you're just confirming that the notification was triggered, you can also use a sync.WaitGroup and block on completion of the group. I found the WaitGroup approach to be less flaky when testing with goroutines.
| } | ||
|
|
||
| func TestNewPollingProjectConfigManagerPullImmediatelyOnStart(t *testing.T) { | ||
| m := sync.RWMutex{} |
There was a problem hiding this comment.
Similar comment here about using the Mutex as opposed to other synchronization options.
| type AtomicManager struct { | ||
| handlers map[uint32]func(interface{}) | ||
| counter uint32 | ||
| lock sync.Mutex |
There was a problem hiding this comment.
Should this be a RWMutex?
| for _, handler := range am.handlers { | ||
| handler(notification) | ||
| } |
There was a problem hiding this comment.
Per our group discussion, we can first create an array of handlers, outside of the map, then trigger the handlers. That way we are not potentially modifying the Map while we're iterating.
5 participants
Summary
ProjectConfigon initializationNewPollingConfigManagerTest plan
Issues
sdk-key, should have the code to block execution untilProjectConfigis available or registerOnProjectConfigUpdatenotification