golang-lru
golang-lru copied to clipboard
hashicorp
Fix confusing error msg. during 2Q creation
For cases where the combination of 2Q size and ghostRatio result in evict cache of size 0, the error message displayed to the user is rather confusing.
This commit proposes to fix the code and give a suitable error message to the user.
Addresses: Issue #51
The error text you've added assumes the cause of errors but the error is coming from a different package. The better approach would be to go into the simplelru package and update the errors there, then keep this code the same, passing through the better error messages.
@jefferai thanks for your comment but IMO my solution is correct. Reasons:
- There is no need to change anything in simplelru package because the package gives a proper reason for the error which is quite simple:
the size argument is not a positive number. If we were to use simplelru package on its own (like this for eg) then I think the error message quite clearly explains the problem. All the extra error information should be added in the calling/downstream code since that is the place where extra context/information is available. - By looking at
simplelru/lru.gocode we can see that only time an error can occur is while creating a new simple lru whensize <= 0, hence the caller can safely assume that whensimplelru.NewLRU()returns a non-nil error that can only mean one thing: the caller passed an invalid size.
PS: Ideally the simplelru package could implement a custom error type for eg simplelru.InvalidSize which the calling/downstream code can check via type inference and act accordingly. But since this is a simple package and not many errors are occurring, we can ignore this for now
By looking at simplelru/lru.go code we can see that only time an error can occur is while creating a new simple lru when size <= 0, hence the caller can safely assume that when simplelru.NewLRU() returns a non-nil error that can only mean one thing: the caller passed an invalid size.
This is true now but may not be true in the future. This is why you should pass the error message through.
@jefferai if I change the error message in simplelru.NewLRU() to "Failed to create Evict cache, please choose a higher 'ghostRatio' or increase cache 'size'" then the caller who is only using simplelru package will get a rather confusing error message talking about some ghostRatio which does not exist in simplelru.
New proposal: I will implement a new error type simplelru.InvalidSize which will allow us to concretely identify the cause of error without breaking simplelru api.