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
use defaults suggested in docs explicitly in code #178
base: master
Are you sure you want to change the base?
Conversation
@sadqx thanks for the review! Sorry, the CLA somehow flew under the radar. All green now! 👍 |
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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