Add a helper method to set eviction function after construction #318
Conversation
k8s-ci-robot
added
kind/feature
k8s-ci-robot
added
approved
| @@ -44,6 +44,11 @@ func NewWithEvictionFunc(size int, f EvictionFunc) *Cache { | |||
| return c | |||
| } | |||
|
|
|||
| // SetEvictionFunc updates the eviction func | |||
| func (c *Cache) SetEvictionFunc(f EvictionFunc) { | |||
| c.cache.OnEvicted = f | |||
There was a problem hiding this comment.
should be thread safe and lock before setting the value?
same for NewWithEvictionFunc?
There was a problem hiding this comment.
good point! added it here. But we don't need it in NewWithEvictionFunc as we do not return the internal Cache we create for the user to use until the method exits, so setting c.cache.OnEvicted = f without the locks are fine.
There was a problem hiding this comment.
good point! added it here
was that added in a follow-up? not seeing it here
There was a problem hiding this comment.
@liggitt would you recommend both the lock/unlock in addition to the "return error if it is already set"? i can cut another PR if so
There was a problem hiding this comment.
@liggitt would you recommend both the lock/unlock in addition to the "return error if it is already set"?
yeah... going from nil to set still requires a lock since the function can be read/used concurrently from other methods
There was a problem hiding this comment.
(and disallowing resetting it once set still sgtm)
|
It seems correct, but it will be nice if you can just add an unit test, looking at this https://github.com/kubernetes/utils/pull/275/files it seems you can just copy paste that but instead of using the constructor you use this new helper |
Signed-off-by: Davanum Srinivas <davanum@gmail.com>
dims
force-pushed
the
add-a-helper-method-to-set-eviction-function-after-construction
branch
from
8c2389e
to
7edc6f1
Compare
k8s-ci-robot
added
size/M
indeed! i tightened the api a bit, by allowing the set function only once as well. added tests. |
|
/lgtm Thanks |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, dims The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
We have a use case in kubernetes/kubernetes#128507 where we set
OnEvictedlater.https://github.com/dims/utils/pull/new/add-a-helper-method-to-set-eviction-function-after-construction
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note: