Feature/tiered storage lru policy#960
Conversation
foodprocessor
left a comment
foodprocessor
left a comment
There was a problem hiding this comment.
Very good work! Good design, solid choices.
The function pointers are a very clever way to maintain modularity. Nice touch!
|
|
||
| //Functions to wire later into tiered_storage package | ||
| //upload function from tiered storage WIRE THIS LATER in tiered storage because we are using a function from there | ||
| upload func(name string) error |
There was a problem hiding this comment.
Since this is sort of a function pointer, should we name it uploadFunction or uploadFn or something?
| // eviction | ||
|
|
||
| //check du , do stat file before, based on difference between DU and | ||
|
|
||
| //1. check if we need eviction | ||
| curSize, err := common.GetUsage(q.cachePath) | ||
| if err != nil { | ||
| log.Err("lruPolicy::capacityChecker : failed to get usage: %v", err) | ||
| continue | ||
| } | ||
| if curSize/q.maxCacheSize <= q.threshold { | ||
| break | ||
| } | ||
|
|
||
| //targetRatio should always be less than thresholdRatio | ||
|
|
||
| //find difference to evict down to 60% | ||
| difference := curSize - q.maxCacheSize*q.targetRatio | ||
| curEvictedSpace := 0 | ||
| for curEvictedSpace < int(difference) { | ||
| nodeSize, evicted := q.eviction() | ||
| if !evicted { | ||
| break | ||
| } | ||
| curEvictedSpace += int(nodeSize) | ||
| } |
There was a problem hiding this comment.
This looks good!
Later on, when we're looking at concurrency, let's make sure this code can't run more than once at a time (if the ticker is faster than eviction, we don't begin another eviction loop in parallel).
| q.extractNode(nodeToEvict) | ||
| q.nodeMap.Delete(nodeToEvict.name) |
There was a problem hiding this comment.
Is there a rule we want to follow, for what node membership in the map and the linked list mean? In other words, should we update our records and then execute the action (upload & delete), or visa versa? Which is better for error handling?
| //create node | ||
| newNode := &lruNode{name: name} | ||
| val, found := q.nodeMap.LoadOrStore(name, newNode) | ||
| node := val.(*lruNode) | ||
|
|
||
| if found { | ||
| // touch | ||
| q.extractNode(node) | ||
| } else { | ||
| // brand new node — update tail if list was empty | ||
| if q.tail == nil { | ||
| q.tail = node | ||
| } | ||
| } | ||
| q.setHead(node) |
There was a problem hiding this comment.
This creates a new node before searching for an existing one. It still does what we expect in the end, but it smells off.
| //create node | ||
| newNode := &lruNode{name: name} | ||
| val, found := q.nodeMap.LoadOrStore(name, newNode) | ||
| node := val.(*lruNode) |
There was a problem hiding this comment.
Also, dereferencing / type asserting val before we know if val was found makes me nervous. I figure it's probably fine and just returns nil in practice, but from my experience with C, I see echoes of "nil pointer dereference" here.
2 participants
What type of Pull Request is this? (check all applicable)
Describe your changes in brief
LRU initial implementation
Checklist
Related Issues