embedded cache refactoring #10989

vlad-diachenko · 2023-10-21T21:27:01Z

What this PR does / why we need it:
refactored embedded cache to allow using it for any types, not only for string -> []byte.
Also, the new implementation allows passing the callback that will be called for the removed entry.

…tring to []byte. Also, the new implementation allows passing the callback that will be called for the removed entry. Signed-off-by: Vladyslav Diachenko <[email protected]>

dannykopping

Very nice work @vlad-diachenko! Left a couple thoughts

dannykopping · 2023-10-22T08:20:23Z

pkg/storage/chunk/cache/embeddedcache.go

-	c.entries = make(map[string]*list.Element)
+	if c.onEntryRemoved != nil {
+		for _, entry := range c.entries {
+			castedEntry := entry.Value.(*cacheEntry[K, V])


Please check that this cast succeeds, otherwise the next line will panic

Hhmm, do we really want to call this function for every item once Stop() is called? That could be quite slow. Is it necessary?

Yes, if the value must be properly closed, we need to call on onEntryRemoved callback. In our case, we will use this function to remove the data from the disk.
It's not always necessary if you run your pod against ephemeral storage, but if you run Loki on our local machine, then we need to clean up the data from the disk.
If it becomes unnecessary for some cases in the future, we can add an additional toggle to disable calling this function during the stop.
wdyt?

What are you storing on disk?

One way you could solve this efficiently is to generate a unique ID, and store all your objects on disk under a directory with that ID. When the cache is Stop()-ed, you can delete the directory.

What are you storing on disk?

we will store extracted bloom-blocks on the disk. and we need to be able to delete the directory with this block if entry is evicted from the cache.

One way you could solve this efficiently is to generate a unique ID, and store all your objects on disk under a directory with that ID. When the cache is Stop()-ed, you can delete the directory.

yes, we could do it but it will require having different callbacks, one for onEntryRemoved and one more for onStop... anyway, for now, such cache with onEntryRemoved callback will be used only for bloom-blocks so it does not affect rest of the implementation because the callback will be nil there...
Also, even if it will be a thousand of the directories to delete, it should not take a lot of time.
wdyt?

Why not just use memcached from the beginning? memcached is an LRU...

I am not sure what will be better to store the blocks on the disk or download them from the Memcache every time. we expect these blocks to be big enough, so we want to store them on a local disk because it might be just faster...
Let's say if the query touches 100GB of data, then it might be necessary to download about 1 GB of blocks, it might take some time ...

If you want, we can jump on a call tomorrow or later today to discuss.

yep, let's discuss it tomorrow

removed execution onEntryRemoved callback during the cache stop a6f7a43

pkg/storage/chunk/cache/embeddedcache.go

chaudum · 2023-10-23T07:54:04Z

pkg/storage/chunk/cache/embeddedcache.go

 	c.lock.Lock()
 	defer c.lock.Unlock()

 	for k, v := range c.entries {
-		entry := v.Value.(*cacheEntry)
+		entry := v.Value.(*cacheEntry[K, V])


Even though the cache value should not be of a different type, we should still check the cast result:

Suggested change

entry := v.Value.(*cacheEntry[K, V])

entry, ok := v.Value.(*cacheEntry[K, V])

if ok && time.Since(entry.updated) > ttl {

c.remove(k, v, expiredReason)

}

asked question below ;)

If the type is not expected, a panic seems appropriate

vlad-diachenko · 2023-10-23T13:27:06Z

hey @chaudum , @dannykopping
. I have a question about the need to assert the type of entry from the cache.
The underlying cache is a private field, so, nobody can put the entries from outside and currently, it’s only possible to do using Store function that forces the entries to have types [K, V] .
I am not sure that we need to assert if the type is casted properly because it means that somebody produced the bug in the cache itself, and this bug also is not caught by the test…. and I do not know if the check of the type will help us here…. maybe it’s better to panic because in this case, I do not think that application can operate
wdyt?

vlad-diachenko · 2023-10-24T09:27:07Z

@chaudum @dannykopping what do you think about the PR? can we merge it?

Signed-off-by: Vladyslav Diachenko <[email protected]>

**What this PR does / why we need it**: refactored embedded cache to allow using it for any types, not only for `string -> []byte`. Also, the new implementation allows passing the callback that will be called for the removed entry. --------- Signed-off-by: Vladyslav Diachenko <[email protected]>

refactored embedded cache to allow using it for any types, not only s…

5c59c73

…tring to []byte. Also, the new implementation allows passing the callback that will be called for the removed entry. Signed-off-by: Vladyslav Diachenko <[email protected]>

vlad-diachenko requested a review from a team as a code owner October 21, 2023 21:27

pull-request-size bot added the size/L label Oct 21, 2023

dannykopping reviewed Oct 22, 2023

View reviewed changes

chaudum reviewed Oct 23, 2023

View reviewed changes

vlad-diachenko requested review from chaudum and dannykopping October 24, 2023 09:27

vlad-diachenko added 2 commits October 25, 2023 10:05

removed execution onEntryRemoved callback during the cache stop

a6f7a43

Signed-off-by: Vladyslav Diachenko <[email protected]>

Merge branch 'main' into vlad.diachenko/embedded-cache-refactoring

0c1e34a

vlad-diachenko enabled auto-merge (squash) October 25, 2023 07:16

Merge branch 'main' into vlad.diachenko/embedded-cache-refactoring

2dbde8b

vlad-diachenko disabled auto-merge October 26, 2023 14:34

owen-d approved these changes Oct 26, 2023

View reviewed changes

Merge branch 'main' into vlad.diachenko/embedded-cache-refactoring

ccb1da5

vlad-diachenko enabled auto-merge (squash) November 1, 2023 14:53

vlad-diachenko merged commit 45b10ef into main Nov 1, 2023
3 checks passed

vlad-diachenko deleted the vlad.diachenko/embedded-cache-refactoring branch November 1, 2023 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

embedded cache refactoring #10989

embedded cache refactoring #10989

vlad-diachenko commented Oct 21, 2023

dannykopping left a comment

dannykopping Oct 22, 2023

dannykopping Oct 22, 2023 •

edited

Loading

vlad-diachenko Oct 23, 2023

dannykopping Oct 23, 2023 •

edited

Loading

vlad-diachenko Oct 23, 2023

dannykopping Oct 24, 2023

vlad-diachenko Oct 24, 2023

dannykopping Oct 24, 2023

vlad-diachenko Oct 24, 2023

vlad-diachenko Oct 25, 2023

chaudum Oct 23, 2023

vlad-diachenko Oct 23, 2023

dannykopping Oct 23, 2023

vlad-diachenko commented Oct 23, 2023

vlad-diachenko commented Oct 24, 2023

-		entry := v.Value.(*cacheEntry[K, V])
+		entry, ok := v.Value.(*cacheEntry[K, V])
+		if ok && time.Since(entry.updated) > ttl {
+		    c.remove(k, v, expiredReason)
+		}

embedded cache refactoring #10989

embedded cache refactoring #10989

Conversation

vlad-diachenko commented Oct 21, 2023

dannykopping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannykopping Oct 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannykopping Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlad-diachenko commented Oct 23, 2023

vlad-diachenko commented Oct 24, 2023

dannykopping Oct 22, 2023 •

edited

Loading

dannykopping Oct 23, 2023 •

edited

Loading