Skip to content
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

Fix confusing error msg. during 2Q creation #60

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rsjethani
Copy link

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

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 hashicorp#51
@hashicorp-cla
Copy link

hashicorp-cla commented Sep 1, 2019

CLA assistant check
All committers have signed the CLA.

@jefferai
Copy link
Member

jefferai commented Sep 3, 2019

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.

@rsjethani
Copy link
Author

@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.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.

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

@jefferai
Copy link
Member

jefferai commented Sep 5, 2019

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.

@rsjethani
Copy link
Author

@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.

@rsjethani
Copy link
Author

@jefferai what you think of the updated changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants