From 95a115c985c358561aebb4398d963c49feece5f4 Mon Sep 17 00:00:00 2001 From: Jusshersmith Date: Wed, 20 Nov 2019 15:47:47 +0000 Subject: [PATCH 1/7] fillcache trigger immediately --- internal/pkg/groups/fillcache.go | 45 ++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 16 deletions(-) diff --git a/internal/pkg/groups/fillcache.go b/internal/pkg/groups/fillcache.go index 9e8578be..6477f7e5 100644 --- a/internal/pkg/groups/fillcache.go +++ b/internal/pkg/groups/fillcache.go @@ -115,7 +115,12 @@ func (c *FillCache) RefreshLoop(group string) bool { c.refreshLoopGroups[group] = struct{}{} c.mu.Unlock() + // we want the cache to be refreshed once immediately, + // and then again upon each tick. so we create a channel and + // send to it once, triggering a refresh straight away. + triggerOnceCh := make(chan bool) ticker := time.NewTicker(c.refreshTTL) + go func() { // cleanup if this goroutine exits defer func() { @@ -125,29 +130,37 @@ func (c *FillCache) RefreshLoop(group string) bool { }() for { + logger := log.NewLogEntry() select { case <-c.stopCh: return + case <-triggerOnceCh: + break case <-ticker.C: - logger := log.NewLogEntry() - - logger.WithUserGroup(group).Info("updating fill cache") - updated, err := c.Update(group) - if err != nil { - c.StatsdClient.Incr("groups_cache.error", - []string{ - fmt.Sprintf("group:%s", group), - fmt.Sprintf("error:%s", err), - }, 1.0) - logger.WithUserGroup(group).Error( - err, "error updating fill cache") - } - if !updated { - logger.WithUserGroup(group).Info("cache was not updated") - } + break + } + + logger.WithUserGroup(group).Info("updating fill cache") + updated, err := c.Update(group) + if err != nil { + c.StatsdClient.Incr("groups_cache.error", + []string{ + fmt.Sprintf("group:%s", group), + fmt.Sprintf("error:%s", err), + }, 1.0) + logger.WithUserGroup(group).Error( + err, "error updating fill cache") + } + if !updated { + logger.WithUserGroup(group).Info("cache was not updated") } } }() + + // trigger initial cache refresh + triggerOnceCh <- true + // closing seems to cause it to be triggered every second or so + //close(triggerOnceCh) return true } From ee16ad0241582c7ebcf60afb0988f700f03018bb Mon Sep 17 00:00:00 2001 From: Jusshersmith Date: Thu, 21 Nov 2019 12:47:59 +0000 Subject: [PATCH 2/7] move to new function --- internal/pkg/groups/fillcache.go | 55 +++++++++++++++----------------- 1 file changed, 26 insertions(+), 29 deletions(-) diff --git a/internal/pkg/groups/fillcache.go b/internal/pkg/groups/fillcache.go index 6477f7e5..da36c4c9 100644 --- a/internal/pkg/groups/fillcache.go +++ b/internal/pkg/groups/fillcache.go @@ -93,6 +93,27 @@ func (c *FillCache) Update(group string) (bool, error) { return false, err } +// update wraps the Update method, adding instrumentation +func (c *FillCache) update(group string) { + logger := log.NewLogEntry() + logger.WithUserGroup(group).Info("updating fill cache") + + updated, err := c.Update(group) + if err != nil { + c.StatsdClient.Incr("groups_cache.error", + []string{ + fmt.Sprintf("group:%s", group), + fmt.Sprintf("error:%s", err), + }, 1.0) + logger.WithUserGroup(group).Error( + err, "error updating fill cache") + } + + if !updated { + logger.WithUserGroup(group).Info("cache was not updated") + } +} + // RefreshLoop runs in a separate goroutine for the key in the cache and // updates the cache value for that key every refreshTTL func (c *FillCache) RefreshLoop(group string) bool { @@ -115,12 +136,7 @@ func (c *FillCache) RefreshLoop(group string) bool { c.refreshLoopGroups[group] = struct{}{} c.mu.Unlock() - // we want the cache to be refreshed once immediately, - // and then again upon each tick. so we create a channel and - // send to it once, triggering a refresh straight away. - triggerOnceCh := make(chan bool) ticker := time.NewTicker(c.refreshTTL) - go func() { // cleanup if this goroutine exits defer func() { @@ -129,38 +145,19 @@ func (c *FillCache) RefreshLoop(group string) bool { c.mu.Unlock() }() + // we update the cache once before looping to ensure the cache is filled immediately, + // instead of only after c.refreshTTL + c.update(group) + for { - logger := log.NewLogEntry() select { case <-c.stopCh: return - case <-triggerOnceCh: - break case <-ticker.C: - break - } - - logger.WithUserGroup(group).Info("updating fill cache") - updated, err := c.Update(group) - if err != nil { - c.StatsdClient.Incr("groups_cache.error", - []string{ - fmt.Sprintf("group:%s", group), - fmt.Sprintf("error:%s", err), - }, 1.0) - logger.WithUserGroup(group).Error( - err, "error updating fill cache") - } - if !updated { - logger.WithUserGroup(group).Info("cache was not updated") + c.update(group) } } }() - - // trigger initial cache refresh - triggerOnceCh <- true - // closing seems to cause it to be triggered every second or so - //close(triggerOnceCh) return true } From 8b70c3704ad6bfe04c4c48ab480baf14fbf34ead Mon Sep 17 00:00:00 2001 From: Jusshersmith Date: Thu, 21 Nov 2019 16:22:10 +0000 Subject: [PATCH 3/7] pull instrumentation into same method --- internal/pkg/groups/fillcache.go | 49 ++++++++++++++------------------ 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/internal/pkg/groups/fillcache.go b/internal/pkg/groups/fillcache.go index da36c4c9..2a0604f4 100644 --- a/internal/pkg/groups/fillcache.go +++ b/internal/pkg/groups/fillcache.go @@ -19,7 +19,7 @@ type MemberSetCache interface { // Get returns a MemberSet from the cache Get(string) (MemberSet, bool) // Update updates the MemberSet of a given key, return a boolean updated value, and and error - Update(string) (bool, error) + Update(string) bool // RefreshLoop starts an update refresh loop for a given key and returns a boolean value of it was started RefreshLoop(string) bool // Stop is a function to stop all goroutines that may have been spun up for the cache. @@ -71,11 +71,14 @@ func (c *FillCache) Get(group string) (MemberSet, bool) { // Update recomputes the value for the given key, unless another goroutine is // already computing the value, and returns a bool indicating whether the value // was updated -func (c *FillCache) Update(group string) (bool, error) { +func (c *FillCache) Update(group string) bool { + logger := log.NewLogEntry() + logger.WithUserGroup(group).Info("updating fill cache") + c.mu.Lock() if _, waiting := c.inflight[group]; waiting { c.mu.Unlock() - return false, nil + return false } c.inflight[group] = struct{}{} @@ -88,30 +91,16 @@ func (c *FillCache) Update(group string) (bool, error) { if err == nil { c.cache[group] = val - return true, nil - } - return false, err -} - -// update wraps the Update method, adding instrumentation -func (c *FillCache) update(group string) { - logger := log.NewLogEntry() - logger.WithUserGroup(group).Info("updating fill cache") - - updated, err := c.Update(group) - if err != nil { - c.StatsdClient.Incr("groups_cache.error", - []string{ - fmt.Sprintf("group:%s", group), - fmt.Sprintf("error:%s", err), - }, 1.0) - logger.WithUserGroup(group).Error( - err, "error updating fill cache") + return true } - if !updated { - logger.WithUserGroup(group).Info("cache was not updated") - } + c.StatsdClient.Incr("groups_cache.error", + []string{ + fmt.Sprintf("group:%s", group), + fmt.Sprintf("error:%s", err), + }, 1.0) + logger.WithUserGroup(group).Error(err, "error updating fill cache") + return false } // RefreshLoop runs in a separate goroutine for the key in the cache and @@ -147,14 +136,20 @@ func (c *FillCache) RefreshLoop(group string) bool { // we update the cache once before looping to ensure the cache is filled immediately, // instead of only after c.refreshTTL - c.update(group) + updated := c.Update(group) + if !updated { + logger.WithUserGroup(group).Info("cache was not updated") + } for { select { case <-c.stopCh: return case <-ticker.C: - c.update(group) + updated = c.Update(group) + if !updated { + logger.WithUserGroup(group).Info("cache was not updated") + } } } }() From d3b386f52a6ece42dc2f22ca2a960b0a46c34576 Mon Sep 17 00:00:00 2001 From: Jusshersmith Date: Thu, 21 Nov 2019 16:26:05 +0000 Subject: [PATCH 4/7] logger --- internal/pkg/groups/fillcache.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/pkg/groups/fillcache.go b/internal/pkg/groups/fillcache.go index 2a0604f4..29ef5f4e 100644 --- a/internal/pkg/groups/fillcache.go +++ b/internal/pkg/groups/fillcache.go @@ -127,6 +127,7 @@ func (c *FillCache) RefreshLoop(group string) bool { ticker := time.NewTicker(c.refreshTTL) go func() { + logger := log.NewLogEntry() // cleanup if this goroutine exits defer func() { c.mu.Lock() From 55c8d0edbe873b635b5695e40221ca73066b1037 Mon Sep 17 00:00:00 2001 From: Jusshersmith Date: Thu, 21 Nov 2019 16:39:44 +0000 Subject: [PATCH 5/7] cache tests --- internal/pkg/groups/fillcache_test.go | 25 +++++++++---------------- internal/pkg/groups/mock_cache.go | 5 ++--- 2 files changed, 11 insertions(+), 19 deletions(-) diff --git a/internal/pkg/groups/fillcache_test.go b/internal/pkg/groups/fillcache_test.go index e4b273ba..e1075adc 100644 --- a/internal/pkg/groups/fillcache_test.go +++ b/internal/pkg/groups/fillcache_test.go @@ -14,34 +14,27 @@ func testFillFunc(members MemberSet, fillError error) func(string) (MemberSet, e func TestFillCacheUpdate(t *testing.T) { testCases := []struct { - name string - members MemberSet - fillError error - updated bool - expectedError bool + name string + members MemberSet + fillError error + updated bool }{ { - name: "update to empty cache, no fill errors", + name: "successful update to empty cache", members: MemberSet{"a": {}, "b": {}, "c": {}}, updated: true, }, { - name: "update with fill function error", - fillError: fmt.Errorf("fill error"), - expectedError: true, + name: "unsuccessful update to cache", + fillError: fmt.Errorf("fill error"), + updated: false, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { fillCache := NewFillCache(testFillFunc(tc.members, tc.fillError), time.Hour) defer fillCache.Stop() - ok, err := fillCache.Update("groupKey") - if err == nil && tc.expectedError { - t.Errorf("expected error but err was nil") - } - if err != nil && !tc.expectedError { - t.Errorf("unexpected error %s", err) - } + ok := fillCache.Update("groupKey") if tc.updated != ok { t.Errorf("expected updated to be %v but was %v", tc.updated, ok) } diff --git a/internal/pkg/groups/mock_cache.go b/internal/pkg/groups/mock_cache.go index 9b22be9b..2a9e905f 100644 --- a/internal/pkg/groups/mock_cache.go +++ b/internal/pkg/groups/mock_cache.go @@ -5,7 +5,6 @@ type MockCache struct { ListMembershipsFunc func(string) (MemberSet, bool) Exists bool Updated bool - UpdateError error Refreshed bool } @@ -15,8 +14,8 @@ func (mc *MockCache) Get(group string) (MemberSet, bool) { } // Update updates the cache -func (mc *MockCache) Update(string) (bool, error) { - return mc.Updated, mc.UpdateError +func (mc *MockCache) Update(string) bool { + return mc.Updated } // RefreshLoop returns a boolean of if the refresh loop is refreshed From 834f70eb6e8639e511c6a68481ea212c17c41373 Mon Sep 17 00:00:00 2001 From: Jusshersmith Date: Fri, 22 Nov 2019 18:19:14 +0000 Subject: [PATCH 6/7] add quick test --- internal/pkg/groups/fillcache_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/internal/pkg/groups/fillcache_test.go b/internal/pkg/groups/fillcache_test.go index e1075adc..408b94b6 100644 --- a/internal/pkg/groups/fillcache_test.go +++ b/internal/pkg/groups/fillcache_test.go @@ -64,11 +64,22 @@ func TestRefreshLoop(t *testing.T) { if tc.refreshLoopGroups != nil { fillCache.refreshLoopGroups = tc.refreshLoopGroups } + started := fillCache.RefreshLoop("group1") + if tc.expectedStarted != started { t.Errorf("expected started to be %v but was %v", tc.expectedStarted, started) } + if tc.expectedStarted { + // wait briefly to allow the cache to be updated + time.Sleep(50 * time.Millisecond) + _, ok := fillCache.Get("group1") + if !ok { + t.Errorf("expected the group cache to be updated immediately") + } + } + }) } From 7165315a9f38b76215294fee5f8843ef7b700a80 Mon Sep 17 00:00:00 2001 From: Jusshersmith Date: Wed, 27 Nov 2019 15:39:56 +0000 Subject: [PATCH 7/7] update comments --- internal/pkg/groups/fillcache.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/pkg/groups/fillcache.go b/internal/pkg/groups/fillcache.go index 29ef5f4e..0b77e47e 100644 --- a/internal/pkg/groups/fillcache.go +++ b/internal/pkg/groups/fillcache.go @@ -18,9 +18,9 @@ const ( type MemberSetCache interface { // Get returns a MemberSet from the cache Get(string) (MemberSet, bool) - // Update updates the MemberSet of a given key, return a boolean updated value, and and error + // Update updates the MemberSet of a given key and returns a boolean value indicating whether the value was updated or not. Update(string) bool - // RefreshLoop starts an update refresh loop for a given key and returns a boolean value of it was started + // RefreshLoop starts an update refresh loop for a given key and returns a boolean value indicating whether a refresh loop has been started or not RefreshLoop(string) bool // Stop is a function to stop all goroutines that may have been spun up for the cache. Stop()