use defaults suggested in docs explicitly in code #178
Conversation
costela
requested review from
all-seeing-code,
karlmcguire,
manishrjain and
martinmr
as code owners
|
|
costela
removed request for
martinmr,
manishrjain,
all-seeing-code and
karlmcguire
|
@sadqx thanks for the review! Sorry, the CLA somehow flew under the radar. All green now! 👍 |
There was a problem hiding this comment.
@costela , thank you for suggesting the change and working on the improvement. I have added a few comments inline and would appreciate it if you can look at those.
| } | ||
|
|
||
| if config.NumCounters == 0 { | ||
| config.NumCounters = config.MaxCost * defaultNumCountersMult // sensible default |
There was a problem hiding this comment.
Since MaxCost is the upper bound on size of cache in Bytes, it doesn't necessarily approximate to max number of keys we want in it. This likely leads to over-approximation for value of NumCounters which should be 10 times the number of keys in cache and not 10 times the size of cache.
There was a problem hiding this comment.
True, but I assumed over-approximation would be less harmful then under-approximation. Isn't this the case?
| @@ -133,7 +133,7 @@ func TestNewCache(t *testing.T) { | |||
| MaxCost: 10, | |||
| BufferItems: 0, | |||
| }) | |||
| require.Error(t, err) | |||
There was a problem hiding this comment.
Is it possible to add more test cases pertinent to the changes in defaulting behavior? This is to make sure that any future changes to this behavior are rightly caught by the test case(s)
There was a problem hiding this comment.
I just tried making these tests a bit more explicit. PTAL
I don't think we can significantly improve coverage further here: we can't realistically check for all cases where we don't expect an error, so we'd bound to covering the error cases. WDYT?
| @@ -133,7 +133,7 @@ func TestNewCache(t *testing.T) { | |||
| MaxCost: 10, | |||
| BufferItems: 0, | |||
| }) | |||
| require.Error(t, err) | |||
There was a problem hiding this comment.
Is it possible to add more test cases pertinent to the changes in defaulting behavior? This is to make sure that any future changes to this behavior are rightly caught by the test case(s)
Please see this duscussion for a rationale for this suggestion.
This change is