-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove core restriction in cache and turn it into an active/standby restriction instead #3849
Conversation
restriction instead.
vault/core.go
Outdated
purgable.Purge(c.activeContext) | ||
} | ||
c.physicalCache.Purge(c.activeContext) | ||
c.physicalCache.SetEnabled(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to conditionally enable it based on if we have caching disabled in the config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops yes you're right. I thought about that before and then forgot to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
physical/physical.go
Outdated
// Purgable is an optional interface for backends that support | ||
// purging of their caches. | ||
type Purgable interface { | ||
// An interface for backends that can toggle on or off special functionality |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit, but it should start with "ToggleablePurgemonster is ..."
physical/inmem/cache_test.go
Outdated
@@ -63,268 +64,3 @@ func TestCache_Purge(t *testing.T) { | |||
t.Fatalf("should not have key") | |||
} | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add test cases for when cache.SetEnabled(false) to make sure operations are not getting/modifying from cached values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add such a test, good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
backend: b, | ||
lru: cache, | ||
locks: locksutil.CreateLocks(), | ||
logger: logger, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enabled
seems to be false by default (meaning no cache), is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Fail safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, in that case perhaps adding a comment on the need to call SetEnabled before the Cache really starts caching would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
physical/physical.go
Outdated
// Purgable is an optional interface for backends that support | ||
// purging of their caches. | ||
type Purgable interface { | ||
// ToggleablePurgemonsteris an interface for backends that can toggle on or off |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/ToggleablePurgemonsteris/ToggleablePurgemonster is/
ae095fd
No description provided.