fix: group.Set() not updating hot cache for all nodes#69
fix: group.Set() not updating hot cache for all nodes#69xWiiLLz wants to merge 4 commits intomailgun:masterfrom
Conversation
| if e != nil { | ||
| err = errors.Join(err, e) | ||
| } |
There was a problem hiding this comment.
If there were some errors but the last one yielded nil, the previous code would not return any error
Baliedge
left a comment
Baliedge
left a comment
There was a problem hiding this comment.
Thank you for this contribution! Very sorry nobody noticed the PR for so long. This is definitely a problem worth fixing. Please see my comments.
|
|
||
| g.localSet(key, value, expire, &g.hotCache) | ||
|
|
||
| for _, peer := range g.peers.GetAll() { |
There was a problem hiding this comment.
I'd like to see this code deduplicated with its counterpart in the owner code path (line 324). Move to function?
| group.localSet(*out.Key, out.Value, expire, &group.mainCache) | ||
| c := &group.mainCache | ||
| if out.HotCache != nil && *out.HotCache { | ||
| c = &group.hotCache |
There was a problem hiding this comment.
I'm thinking that Set() on owner peer should update only mainCache, otherwise respect out.HotCache flag.
| var cmds []*exec.Cmd | ||
| var wg sync.WaitGroup | ||
| for i := 0; i < nChild; i++ { | ||
| cmd := exec.Command(os.Args[0], |
There was a problem hiding this comment.
Not on board with launching nested go test processes. Can this be achieved with goroutines?
There was a problem hiding this comment.
due to how this older version of group cache works, you have to do it this way. This is one of the reasons why I forked and re-wrote it https://github.com/groupcache/groupcache-go ✨
|
@thrawn01 You may want to incorporate this fix in your fork. |
|
I've created a new PR to fix this issue on the new repo groupcache/groupcache-go#11 The correct thing to do is ALWAYS delete the updated key from the hotCache. See the PR for explanation. |
3 participants
When there are more than 2 nodes, the
hotCacheis never updated when using theSet()method. Added a test which triggers the bug (you can test it by removing the accompanying fix done in groupcache.go).However, it does look to me like the
hotCacheparam in the Set method is misleading: the hot caches are used even if the provided arg isfalse, which would then yield out-of-date values to the other nodes 🤔