Fixed hot cache update on calls to Set() #11
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thrawn01
force-pushed
the
thrawn/set-hotcache
branch
from
de04640 to
3548057
Compare
Tochemey
approved these changes
Dec 25, 2024
Baliedge
requested changes
Dec 26, 2024
Collaborator
Baliedge
left a comment
Baliedge
left a comment
There was a problem hiding this comment.
LGTM. Just a little nit to pick.
| ci: tidy lint test | ||
| go mod tidy && git diff --exit-code | ||
| @echo | ||
| @echo "\033[32mEVERYTHING PASSED!\033[0m" |
Collaborator
There was a problem hiding this comment.
Color terminals cannot be assumed.
Contributor
Author
There was a problem hiding this comment.
Seems like this might have been true 10+ years ago, The worst that could happen is they get some escape codes in their output. I don't think we should optimize for the odd chance someone is running this on a TSR compatible terminal.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
@xWiiLLz Reported the hot cache's of remote peers were not updated when calling
Set(). mailgun/groupcache#69This PR fixes a long standing TODO where there was some question if the hot cache should always be updated on calls to
Set().Resolution
We should ALWAYS update the main cache and delete the key from the hot cache when
Set()is called.Rationale
We do not know when the owning peer may switch to a different node due to cluster topography changes. If an owner belongs to a different node the instant after
Set()completes, calls toGet(), consulting the hot cache, may find the previously set value.Because we don't know which or when a node in the cluster may become the owner, every node in the cluster should be treated identically.
I left the
hotCacheboolean in the signature ofSet()as I didn't want to make a version breaking change.