Smarter caches #46

2020-10-22T19:15:42+02:00

lyoshenka commented

2020-10-22 19:15:42 +02:00

(Migrated from github.com)

There are two main additions here:

LRUStore class adds LRU eviction mechanics to any store. This is most useful for finite stores (like disk and memory, which is where we're using it now)
afero.Fs is a nice library for abstracting the filesystem. im using it in DiskStore. makes testing faster.

Tagging Beamer and Andrey because they might find this interesting. It's probably not the last time we'll need to cache something (esp blobs) so let's not reinvent the wheel when it comes to caching.

There are two main additions here: - `LRUStore` class adds LRU eviction mechanics to any store. This is most useful for finite stores (like disk and memory, which is where we're using it now) - `afero.Fs` is a nice library for abstracting the filesystem. im using it in `DiskStore`. makes testing faster. Tagging Beamer and Andrey because they might find this interesting. It's probably not the last time we'll need to cache something (esp blobs) so let's not reinvent the wheel when it comes to caching.

tiger5226 (Migrated from github.com) reviewed 2020-10-22 19:15:42 +02:00

lyoshenka commented

2020-10-22 19:18:06 +02:00

(Migrated from github.com)

Sorry about all the renaming in the PR. Makes it hard to read 🥺

Maybe look at them one commit at a time.

Sorry about all the renaming in the PR. Makes it hard to read :pleading_face: Maybe look at them one commit at a time.

anbsky (Migrated from github.com) reviewed 2020-10-23 07:26:18 +02:00

store/lru.go

					
				@ -0,0 +5,4 @@

					"github.com/lbryio/lbry.go/v2/stream"

					"github.com/lbryio/reflector.go/internal/metrics"

					golru "github.com/hashicorp/golang-lru"

anbsky (Migrated from github.com) commented

2020-10-23 07:26:17 +02:00

We're using a different cache lib in @nikooo777 's proposed cache in lbrytv-player. Is either of them better than the other? Should probably keep it simple and stick to one.

We're using [a different cache lib](https://github.com/karlseguin/ccache) in @nikooo777 's [proposed cache](https://github.com/lbryio/lbrytv-player/pull/25/files) in lbrytv-player. Is either of them better than the other? Should probably keep it simple and stick to one.

anbsky (Migrated from github.com) reviewed 2020-10-23 07:38:06 +02:00

store/lru_test.go

					
				@ -0,0 +10,4 @@

					"github.com/stretchr/testify/assert"

					"github.com/stretchr/testify/require"

				)

anbsky (Migrated from github.com) commented

2020-10-23 07:33:13 +02:00

Given that test functions have to be named Test*, this looks like a typo. createTestLRUstore?

Given that test functions have to be named `Test*`, this looks like a typo. `createTestLRUstore`?

lyoshenka (Migrated from github.com) reviewed 2020-10-23 17:34:31 +02:00

store/lru.go

					
				@ -0,0 +5,4 @@

					"github.com/lbryio/lbry.go/v2/stream"

					"github.com/lbryio/reflector.go/internal/metrics"

					golru "github.com/hashicorp/golang-lru"

lyoshenka (Migrated from github.com) commented

2020-10-23 17:34:30 +02:00

Yep. We're actually planning to merge reflector and player into a single program/process so they don't have to send blobs back and forth via TCP.

lyoshenka (Migrated from github.com) reviewed 2020-10-23 17:35:01 +02:00

store/lru_test.go

					
				@ -0,0 +10,4 @@

					"github.com/stretchr/testify/assert"

					"github.com/stretchr/testify/require"

				)

lyoshenka (Migrated from github.com) commented

2020-10-23 17:35:00 +02:00

agreed

nikooo777 (Migrated from github.com) reviewed 2020-10-26 20:56:36 +01:00

nikooo777 (Migrated from github.com) left a comment

MemStore should probably be removed or replaced with the ccache store.
the LRU store, as andrey mentioned, should not use that library, ccache should do just fine. Also there is probably no need to reload the cache in memory on restart, that was needed in the player back when that was the only kind of cache on the server.

I think the changes are overall good, the charts should be changed when we push this live.

I will test it locally as well to see how it performs.

MemStore should probably be removed or replaced with the ccache store. the LRU store, as andrey mentioned, should not use that library, ccache should do just fine. Also there is probably no need to reload the cache in memory on restart, that was needed in the player back when that was the only kind of cache on the server. I think the changes are overall good, the charts should be changed when we push this live. I will test it locally as well to see how it performs.

cmd/reflector.go

nikooo777 (Migrated from github.com) commented

2020-10-26 20:46:23 +01:00

i'm slightly confused here. what is what?

cmd/reflector.go

nikooo777 (Migrated from github.com) commented

2020-10-26 20:46:48 +01:00

expressed in what?

Sign in to join this conversation.

No reviewers

tiger5226