LFUDA changes #47
No reviewers
Labels
No labels
area: devops
area: discovery
area: docs
area: livestream
area: proposal
consider soon
dependencies
Epic
good first issue
hacktoberfest
help wanted
icebox
level: 1
level: 2
level: 3
level: 4
needs: exploration
needs: grooming
needs: priority
needs: repro
needs: tech design
on hold
priority: blocker
priority: high
priority: low
priority: medium
resilience
Tom's Wishlist
type: bug
type: discussion
type: improvement
type: new feature
type: refactor
type: task
type: testing
unplanned
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: LBRYCommunity/reflector.go#47
Loading…
Reference in a new issue
No description provided.
Delete branch "improvements"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
and other improvements
a bunch of potential changes here. if you think we should put off some for now, we can discuss.
this also needs a clean shutdown. like, what if its putting to the cache and a shutdown happens. we don't want a half-write or somethin.
@ -58,6 +58,7 @@ func TestCachingStore_CacheMiss(t *testing.T) {
if !bytes.Equal(b, res) {
t.Errorf("expected Get() to return %s, got %s", string(b), string(res))
}
time.Sleep(10 * time.Millisecond) //storing to cache is done async so let's give it some time
need a way to do these sync or async. at the very least, an option for the cache. ideally, some process of notifying when writes finish.
seems weird to do this. why not an actual
bool
, or an empty struct?@ -0,0 +9,4 @@
)
// LRUStore adds a max cache size and LRU eviction to a BlobStore
type LFUDAStore struct {
if this is very similar to LRUStore, maybe we need a generic EvictionStrategy struct of some sort
@ -0,0 +62,4 @@
return blob, err
}
// Put stores the blob. Following LFUDA rules it's not guaranteed that a SET will store the value!!!
👍 for the comment
@ -0,0 +76,4 @@
}
// PutSD stores the sd blob. Following LFUDA rules it's not guaranteed that a SET will store the value!!!
func (l *LFUDAStore) PutSD(hash string, blob stream.Blob) error {
why are Put and PutSD implemented differently?
@ -0,0 +103,4 @@
}
// loadExisting imports existing blobs from the underlying store into the LRU cache
func (l *LFUDAStore) loadExisting(store lister, maxItems int) error {
stuff like this makes me even more confident that LRU and LFUDA should be less copy-pasted and more the same object with an eviction strategy selector
use
t.Log()
insteade@ -0,0 +40,4 @@
err = lfuda.Put("two", b)
require.NoError(t, err)
_, err = lfuda.Get("five")
check the blob value too?
@ -48,1 +49,4 @@
func (l *LRUStore) Name() string {
return "lru_" + l.store.Name()
}
this should at least be an option, and ideally should have a way to notify that load finished.
also needs clean shutdown
left over from testing?
the reason I did this was because the library wasn't treating a bool as a 1byte type and it would waste a lot of RAM.
I since worked with the developer of the library to address this so i believe we can go back to using a bool
ah right, this can actually be removed. it was a debug statement
@ -48,1 +49,4 @@
func (l *LRUStore) Name() string {
return "lru_" + l.store.Name()
}
can I argue that a clean shutdown here doesn't matter? or is it just bad?
yes
What's buffer disk cache?
It seems we cannot really "disable" disk cache so this has to be reworded.
@ -48,1 +49,4 @@
func (l *LRUStore) Name() string {
return "lru_" + l.store.Name()
}
If you have a big cache, start this and immediately press
Ctrl+C
, what will happen? My guess is it probably won't shut down and will annoyingly keep running in the foreground.@ -48,1 +49,4 @@
func (l *LRUStore) Name() string {
return "lru_" + l.store.Name()
}
YES! this was in the back of my mind but i forgot about it. i think there's even a TODO in the code. thanks for catching it
disk cache should disable-able. only hot cache is required atm
in fact disk cache should be off by default
also also, i thought i changed this to be the way you described: one flag to enabled and set path, second flag to set size (and there's a default size)
@ -48,1 +49,4 @@
func (l *LRUStore) Name() string {
return "lru_" + l.store.Name()
}
@andybeletsky IIRC as long as the main thread returns, all routines are automatically killed in GO. I'll have to test that but I think a ctrl+c would kill it right away
it's an extra layer of disk caching (meant to be used on a secondary drive, like our extra 500GB nvme drive) to have faster caches. We disabled this in production because it made things worse.
My plan is to get rid of this mess and just use a yaml config file.
Another pair session could be in order to finally get this out...
🧟
Pull request closed