Refactor Get() to work with new faster cache #447
Conversation
jackzampolin
changed the title
|
|
||
| return t.root.get(t, key) | ||
| fastNode, err := t.ndb.GetFastNode(key) |
There was a problem hiding this comment.
Should we log error here? When this operation will fail? Would be good to add a comment. @ValarDragon
There was a problem hiding this comment.
The idea is that if this GetFastNode() call fails for any reason, we assume there is no FastNode equivalent for this key in the nodedb and fall back to the original, yet slower, IAVL logic in place.
I'll add debug logging here and add a comment to make this more clear.
There was a problem hiding this comment.
I think the FastNode is confusing. Maybe we should rename to CachedNode?
|
|
||
| if len(key) == 0 { | ||
| panic("nodeDB.GetFastNode() requires key") | ||
| } |
There was a problem hiding this comment.
please do not panic!
There was a problem hiding this comment.
Good catch Robert, I meant to replace all the panics used with the traditional node logic with errors so that failure to fetch a fast node simply falls back to the original logic in IAVL.
I'll get this taken care of in the next PR
Created a new cache in
nodedb.gothat would be maintained for our newerFastNodestructs and implemented the pieces necessary for changingGet()as per this