LFUDA changes #47

Closed
nikooo777 wants to merge 12 commits from improvements into master
nikooo777 commented 2020-11-27 17:04:01 +01:00 (Migrated from github.com)

and other improvements

and other improvements
lyoshenka (Migrated from github.com) requested changes 2020-11-27 22:36:25 +01:00
lyoshenka (Migrated from github.com) left a comment

a bunch of potential changes here. if you think we should put off some for now, we can discuss.

a bunch of potential changes here. if you think we should put off some for now, we can discuss.
lyoshenka (Migrated from github.com) commented 2020-11-27 22:24:08 +01:00

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.

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
lyoshenka (Migrated from github.com) commented 2020-11-27 22:24:56 +01:00

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.

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.
lyoshenka (Migrated from github.com) commented 2020-11-27 22:26:46 +01:00

seems weird to do this. why not an actual bool, or an empty struct?

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 {
lyoshenka (Migrated from github.com) commented 2020-11-27 22:27:23 +01:00

if this is very similar to LRUStore, maybe we need a generic EvictionStrategy struct of some sort

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!!!
lyoshenka (Migrated from github.com) commented 2020-11-27 22:29:30 +01:00

👍 for the comment

:+1: 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 {
lyoshenka (Migrated from github.com) commented 2020-11-27 22:31:02 +01:00

why are Put and PutSD implemented differently?

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 {
lyoshenka (Migrated from github.com) commented 2020-11-27 22:32:23 +01:00

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

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
lyoshenka (Migrated from github.com) commented 2020-11-27 22:34:03 +01:00

use t.Log() insteade

use `t.Log()` insteade
@ -0,0 +40,4 @@
err = lfuda.Put("two", b)
require.NoError(t, err)
_, err = lfuda.Get("five")
lyoshenka (Migrated from github.com) commented 2020-11-27 22:33:36 +01:00

check the blob value too?

check the blob value too?
@ -48,1 +49,4 @@
func (l *LRUStore) Name() string {
return "lru_" + l.store.Name()
}
lyoshenka (Migrated from github.com) commented 2020-11-27 22:35:00 +01:00

this should at least be an option, and ideally should have a way to notify that load finished.

also needs clean shutdown

this should at least be an option, and ideally should have a way to notify that load finished. also needs clean shutdown
lyoshenka (Migrated from github.com) commented 2020-11-27 22:35:43 +01:00

left over from testing?

left over from testing?
nikooo777 (Migrated from github.com) reviewed 2020-12-09 19:19:19 +01:00
nikooo777 (Migrated from github.com) commented 2020-12-09 19:19:18 +01:00

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

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
nikooo777 (Migrated from github.com) reviewed 2020-12-09 19:20:19 +01:00
nikooo777 (Migrated from github.com) commented 2020-12-09 19:20:19 +01:00

ah right, this can actually be removed. it was a debug statement

ah right, this can actually be removed. it was a debug statement
nikooo777 (Migrated from github.com) reviewed 2020-12-09 19:21:18 +01:00
@ -48,1 +49,4 @@
func (l *LRUStore) Name() string {
return "lru_" + l.store.Name()
}
nikooo777 (Migrated from github.com) commented 2020-12-09 19:21:17 +01:00

can I argue that a clean shutdown here doesn't matter? or is it just bad?

can I argue that a clean shutdown here doesn't matter? or is it just bad?
nikooo777 (Migrated from github.com) reviewed 2020-12-09 19:21:49 +01:00
nikooo777 (Migrated from github.com) commented 2020-12-09 19:21:48 +01:00

yes

yes
anbsky (Migrated from github.com) reviewed 2020-12-10 08:16:18 +01:00
anbsky (Migrated from github.com) commented 2020-12-10 08:16:18 +01:00

What's buffer disk cache?

What's buffer disk cache?
anbsky (Migrated from github.com) reviewed 2020-12-10 08:19:21 +01:00
anbsky (Migrated from github.com) commented 2020-12-10 08:19:20 +01:00

It seems we cannot really "disable" disk cache so this has to be reworded.

It seems we cannot really "disable" disk cache so this has to be reworded.
anbsky (Migrated from github.com) reviewed 2020-12-10 08:24:56 +01:00
@ -48,1 +49,4 @@
func (l *LRUStore) Name() string {
return "lru_" + l.store.Name()
}
anbsky (Migrated from github.com) commented 2020-12-10 08:24:56 +01:00

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.

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.
lyoshenka (Migrated from github.com) reviewed 2020-12-10 15:59:29 +01:00
@ -48,1 +49,4 @@
func (l *LRUStore) Name() string {
return "lru_" + l.store.Name()
}
lyoshenka (Migrated from github.com) commented 2020-12-10 15:59:29 +01:00

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

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
lyoshenka (Migrated from github.com) reviewed 2020-12-10 16:00:23 +01:00
lyoshenka (Migrated from github.com) commented 2020-12-10 16:00:22 +01:00

disk cache should disable-able. only hot cache is required atm

disk cache should disable-able. only hot cache is required atm
lyoshenka (Migrated from github.com) reviewed 2020-12-10 16:00:35 +01:00
lyoshenka (Migrated from github.com) commented 2020-12-10 16:00:35 +01:00

in fact disk cache should be off by default

in fact disk cache should be off by default
lyoshenka (Migrated from github.com) reviewed 2020-12-10 16:01:13 +01:00
lyoshenka (Migrated from github.com) commented 2020-12-10 16:01:13 +01:00

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)

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)
nikooo777 (Migrated from github.com) reviewed 2020-12-22 21:10:14 +01:00
@ -48,1 +49,4 @@
func (l *LRUStore) Name() string {
return "lru_" + l.store.Name()
}
nikooo777 (Migrated from github.com) commented 2020-12-22 21:10:14 +01:00

@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

@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
nikooo777 (Migrated from github.com) reviewed 2020-12-22 21:18:04 +01:00
nikooo777 (Migrated from github.com) commented 2020-12-22 21:18:03 +01:00

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.

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.
kauffj commented 2021-03-10 22:23:18 +01:00 (Migrated from github.com)

Another pair session could be in order to finally get this out...

Another pair session could be in order to finally get this out...
kauffj commented 2021-05-26 22:09:48 +02:00 (Migrated from github.com)

🧟

:zombie:

Pull request closed

Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/reflector.go#47
No description provided.