-
Notifications
You must be signed in to change notification settings - Fork 36
add View() to all the various blockstores. #59
Conversation
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
arc_cache.go
Outdated
return ErrNotFound | ||
} | ||
|
||
if has, _, ok := b.hasCached(k); ok && !has { |
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.
the semantics that if a viewer is set, then the calls to View
will only run View
on items that are in the cache is not fully intuitive.
Is there a reason not to attempt to go back to the underlying store when not cached?
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.
Here the cache responds that it has seen this element and it was confirmed to not exist (cache returned ok && !has
).
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.
We do the same in Get
if you look few lines lower.
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 think naming is the problem. hasCached
naturally read as: "is this item in the cache?" instead of "check if the item is in the has cache".
@willscott this just short circuits the view if we know that the item doesn't exist, because we have a cache entry for it, and it's negative.
@willscott @Kubuxu any more concerns here? Can we move forward and release this, to integrate downstream with a nice version number? |
One comment, but I don't see problems with this as-is |
General process note: for interface changes like this, try to get a signoff from the TL (@aschmahmann in this case) as it may interfere with other plans. I believe this change is safe (and long requested...) but it's still best to be safe. (otherwise, this has been a long time coming and I'm really excited to see how we can use this in go-ipfs to reduce our memory footprint) |
😫