Feature/tiered storage lru policy#960
Conversation
…eature/tiered_storage-lru_policy
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!
| // 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.
jfantinhardesty
left a comment
jfantinhardesty
left a comment
There was a problem hiding this comment.
Great work! This is a strong start to the LRU policy. Just have a few comments.
| } | ||
| nodeSize := fileInfo.Size() | ||
|
|
||
| //remove node from queue and map |
There was a problem hiding this comment.
In the eviction code here, you delete from the LRU, but the file will still remain in the local cache? Do we want the file to stay in the cache or should we delete it when we evict?
| } | ||
|
|
||
| // 6. Eviction, file with open handle, file with no open handle, | ||
| func (suite *lruPolicyTestSuite) TestCapacityCheckerEvictionOpenHandle() { |
There was a problem hiding this comment.
This test is a bit flaky. The sleep could probably be a bit longer. Here is how you can replicate it.
go test -run TestLRUPolicyTestSuite ./component/tiered_storage --tags=unittest,fuse3 -count=50
You should see by running the unit tests 50 times that this test and a few others may fail.
| } | ||
|
|
||
| //find the first applicable node | ||
| for nodeToEvict != nil && q.FileHasOpenFileHandle(nodeToEvict.name) { |
There was a problem hiding this comment.
I think there could be a race condition here. After this check, it is possible that a new request comes in that opens the file before it gets uploaded.
3 participants
What type of Pull Request is this? (check all applicable)
Describe your changes in brief
LRU initial implementation
Checklist
Related Issues