feat: add memory cache #12

lanlou1554 · 2024-03-29T17:29:11Z

Remove DataStore, add DataStoreCache, including DataStore + LRU/LRUK...
Rename LRU/LRUK from cache to replacer
Add memory cache

This resolves #14

xx01cyx · 2024-03-29T17:40:20Z

storage-node/TODO.md

+
+      (Disk cache and memory cache should accept `Bytes` as an input parameter for write_data in this case)
+
+   For me, the first design is very clear, and the second design can save the cost of requesting S3 to get the size. But I think the cost is very low. Maybe the major difference is the API for write_data?


If we're going to send an additional HTTP request for every S3 request to get the file size, then the cost may not be neglectable. But I do vote for the first design cuz it's more straightforward and less prone to bugs...

Due to API constraints, I currently adopt method 2.
As expected, the code becomes very ugly : ( Let me think about how to refine it...

unw9527 · 2024-03-30T00:59:01Z

storage-node/TODO.md

-2. Solve the read-write & write-write conflict for disk operations. Let's say we have 2 requests, and they have same key for the cache, the first request has a cache miss, then it has to go to S3 to fetch data. But now second request comes, and the first request hasn't pulled all the data from S3 and stored them in the disk. So the disk file exists, but the content is not complete. Here is one possible way: when a cache miss happens, we record this key into our cache structure, but with a status `uncompleted`, and when the data is fully pulled and stored, the status is updated to `completed`. When another same request comes and tries to `get` disk file from cache, our cache finds the key exists but its status is `uncompleted`. So the cache should wait until its status turns to `completed`. (The async design to make the cache not simply dry wait is worth discussing)
-3. In async approach, we should support read & output data at the same time, both S3 read & disk write(or network write) and disk read & network write. To support this, we cannot use one single fixed buffer, which is the current implementation of disk_manager. Here is 2 ways to support this: 
+1. For cache policy like `lru.rs` and `lru_k.rs`, we can use fine-grained locks instead of one big lock to lock it. If we use fine-grained locks (at least after getting the value from cache, we don't need to hold its lock), we should add unpin/pin in `lru.rs` and `lru_k.rs`.
+2. Discuss the key for the cache. Currently it is raw S3 request string, but maybe we can make it as `bucket + one key`, since if one key represents a file, then for different requests, keys may overlap, and the current implementation will waste disk space.


Will raw S3 request string be different for the same file? Isn't it something like aws s3 cp s3://bucket-name/path/to/file.txt /local/path/file.txt? In other words, if keys overlap, won't the requests also overlap? (Not sure, maybe wrong)

I left this comment based on the current s3.rs test, so I am not sure what is the raw S3 request string... But if it the same as the s3.rs test (one S3 request string can be converted to one bucket + many keys, and each key represents one file), we should consider the cache key more carefully in the future. I added this todo just to remind us double check the format of raw S3 request string.
Feel free to correct me, since I almost know nothing about S3 requests. I think it depends on one raw S3 request string containing only one file or multiple files.

Will raw S3 request string be different for the same file?

Yes. When we make a request to an S3 object, we must specify its bucket AND its key. So we need separate requests if a table spans multiple keys in a bucket.

Let me correct myself. The raw S3 request string means the input param from storage manager.
So a quick question: for the raw S3 request string in storage manager, can it be transformed to one bucket and multiple keys?

Maybe we have to talk to the catalog team to finalize the data scheme

codecov-commenter · 2024-03-30T23:34:03Z

Codecov Report

Attention: Patch coverage is 92.82297% with 45 lines in your changes are missing coverage. Please review.

Project coverage is 84.68%. Comparing base (724bdd0) to head (2529358).

Files	Patch %	Lines
...rc/cache/data_store_cache/memdisk/cache_manager.rs	87.36%	11 Missing and 23 partials ⚠️
storage-node/src/storage_manager.rs	96.29%	3 Missing and 2 partials ⚠️
storage-node/src/disk/disk_manager.rs	96.29%	1 Missing and 2 partials ⚠️
.../cache/data_store_cache/memdisk/data_store/disk.rs	93.75%	1 Missing ⚠️
...ache/data_store_cache/memdisk/data_store/memory.rs	98.80%	0 Missing and 1 partial ⚠️
storage-node/src/cache/policy/lru_k.rs	95.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #12      +/-   ##
==========================================
+ Coverage   81.70%   84.68%   +2.98%     
==========================================
  Files          15       17       +2     
  Lines        1536     2005     +469     
  Branches     1536     2005     +469     
==========================================
+ Hits         1255     1698     +443     
- Misses        210      215       +5     
- Partials       71       92      +21

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

xx01cyx · 2024-03-31T01:00:46Z

storage-node/src/disk/disk_manager.rs

+    // TOOD(lanlou): Now S3 stream returns small amount of data each time, so it would be expensive
+    // in the current implementation since one disk I/O one S3 stream next (too many disk I/O!)


Compared to chunk_size = 1024, the current S3 stream returns significantly more amounts of data (approx 10^5 bytes per poll)

…he, rename lru cache to lru replacer

storage-node/src/cache/data_store_cache/mod.rs

unw9527 · 2024-03-31T15:21:20Z

I would suggest move that TODO.md to a GitHub issue rather than keeping it as a local file.

lanlou1554 · 2024-03-31T15:44:40Z

I would suggest move that TODO.md to a GitHub issue rather than keeping it as a local file.

Sure, I will do it after I finish this PR.

lanlou1554 · 2024-03-31T21:52:25Z

@unw9527 @xx01cyx
PTAL. Sorry that it is a large PR since I forgot to split the refactor and feature. I think you guys can start reading from storage-node/src/cache/data_store_cache/memdisk/cache_manager.rs.

storage-node/src/cache/policy/mod.rs

storage-node/src/cache/data_store_cache/memdisk/cache_manager.rs

storage-node/src/cache/policy/lru.rs

storage-node/src/cache/policy/mod.rs

storage-node/src/cache/data_store_cache/memdisk/cache_manager.rs

storage-node/src/disk/disk_manager.rs

xx01cyx · 2024-04-01T21:18:40Z

storage-node/src/cache/policy/mod.rs

@@ -50,13 +50,19 @@ impl DataStoreCacheValue for ParpulseDataStoreCacheValue {
 ///
 /// There are different cache policies for the data store cache, such as LRU, LRU-K, etc. See
 /// other files in this module for more details.
-pub trait DataStoreCache {
+pub trait DataStoreReplacer: Send + Sync {


I'm also wondering if there is any use case for the rest of the methods in this trait...

We can refine it in the future!

storage-node/src/cache/data_store_cache/mod.rs

storage-node/src/storage_manager.rs

xx01cyx

Generally LGTM!!

xx01cyx · 2024-04-03T18:43:00Z

storage-node/src/cache/data_store_cache/memdisk/cache_manager.rs

+            if mem_max_file_size > replacer_max_capacity {
+                // TODO: better log.
+                println!("The maximum file size > replacer's max capacity, so we set maximum file size = 1/5 of the maximum capacity.");
+                // By default in this case, replacer can at least 5 files.


Suggested change

// By default in this case, replacer can at least 5 files.

// By default in this case, replacer can store at least 5 files.

Thx a lot yx!!!
我落泪了

xx01cyx reviewed Mar 29, 2024

View reviewed changes

unw9527 reviewed Mar 30, 2024

View reviewed changes

lanlou1554 added 2 commits March 30, 2024 19:26

Add design in TODO.md

2509c10

Merge remote-tracking branch 'origin' into ll/memory_cache

f5e85f1

lanlou1554 force-pushed the ll/memory_cache branch from 51259bd to f5e85f1 Compare March 30, 2024 23:29

Add todo to disk_manager

d7bcc38

xx01cyx reviewed Mar 31, 2024

View reviewed changes

refactor: data_store -> data_store_cache, put lru into data_store_cac…

9f7e9a5

…he, rename lru cache to lru replacer

lanlou1554 force-pushed the ll/memory_cache branch from fcf67ea to 9f7e9a5 Compare March 31, 2024 02:19

feat: add memory cache (todo: add tests)

6d4111f

lanlou1554 commented Mar 31, 2024

View reviewed changes

storage-node/src/cache/data_store_cache/mod.rs Show resolved Hide resolved

delete unnecessary data_store

aed7fb8

Add some comments

4293251

lanlou1554 added 5 commits March 31, 2024 13:35

Add tests for memory and disk_manager

2b1b890

Fix conflicts from branch main

090c72d

Add tests for cache_manager

09c6735

Add tests for storage_manager

b67a81d

Add tests for lru and lruk

9ebb871

lanlou1554 marked this pull request as ready for review March 31, 2024 21:49

Resolve conflict with main

d24bd5e

xx01cyx reviewed Apr 1, 2024

View reviewed changes

storage-node/src/cache/policy/mod.rs Outdated Show resolved Hide resolved

xx01cyx reviewed Apr 1, 2024

View reviewed changes

storage-node/src/cache/data_store_cache/memdisk/cache_manager.rs Outdated Show resolved Hide resolved

unw9527 reviewed Apr 1, 2024

View reviewed changes

storage-node/src/cache/policy/lru.rs Outdated Show resolved Hide resolved

storage-node/src/cache/policy/mod.rs Outdated Show resolved Hide resolved

lanlou1554 added 2 commits April 1, 2024 12:37

Apply comment suggestions

ee7aed2

Remove boolean from put in replacer

b78f7df

xx01cyx reviewed Apr 1, 2024

View reviewed changes

lanlou1554 added 4 commits April 1, 2024 18:24

Apply suggestions from comment

2529358

merge main to this branch

6ee979a

fix format

4e71df2

remove one todo

3a1a538

xx01cyx approved these changes Apr 3, 2024

View reviewed changes

fix typo

e30d8bc

lanlou1554 merged commit d3721ef into main Apr 3, 2024
1 check passed

lanlou1554 deleted the ll/memory_cache branch April 3, 2024 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add memory cache #12

feat: add memory cache #12

lanlou1554 commented Mar 29, 2024 •

edited

Loading

xx01cyx Mar 29, 2024

lanlou1554 Mar 31, 2024

unw9527 Mar 30, 2024

lanlou1554 Mar 30, 2024

xx01cyx Mar 30, 2024

lanlou1554 Mar 30, 2024

xx01cyx Mar 30, 2024

codecov-commenter commented Mar 30, 2024 •

edited

Loading

xx01cyx Mar 31, 2024

unw9527 commented Mar 31, 2024

lanlou1554 commented Mar 31, 2024

lanlou1554 commented Mar 31, 2024

xx01cyx Apr 1, 2024

lanlou1554 Apr 1, 2024

xx01cyx left a comment

xx01cyx Apr 3, 2024

lanlou1554 Apr 3, 2024


		(Disk cache and memory cache should accept `Bytes` as an input parameter for write_data in this case)

		For me, the first design is very clear, and the second design can save the cost of requesting S3 to get the size. But I think the cost is very low. Maybe the major difference is the API for write_data?

		// TOOD(lanlou): Now S3 stream returns small amount of data each time, so it would be expensive
		// in the current implementation since one disk I/O one S3 stream next (too many disk I/O!)

	// By default in this case, replacer can at least 5 files.
	// By default in this case, replacer can store at least 5 files.

feat: add memory cache #12

feat: add memory cache #12

Conversation

lanlou1554 commented Mar 29, 2024 • 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

codecov-commenter commented Mar 30, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

unw9527 commented Mar 31, 2024

lanlou1554 commented Mar 31, 2024

lanlou1554 commented Mar 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xx01cyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lanlou1554 commented Mar 29, 2024 •

edited

Loading

codecov-commenter commented Mar 30, 2024 •

edited

Loading