[ENH] Use foyer for block cache #2431

Ishiihara · 2024-07-01T16:10:59Z

Description of changes

Summarize the changes made by this PR.

Improvements & Bug fixes
- This PR fixes bugs that readers and writers share the cache. The fix is as follows: 1. fork calls get instead of getting data from the read cache 2. commit returns the newly created blocks 3. flush takes the newly created blocks as input.
- Temporarily disable the shuttle test as there is a compatibility issue between tokio filesystem calls and shuttle runtime. Tokio filesystem call will panic if not wrapped in tokio runtime.
New functionality
- This PR introduces cache with eviction policies. Foyer (https://github.com/MrCroxx/foyer) is used as the cache. In the future, Foyer will be used as a hybrid cache.
- Introduced cache-related configs to be used by different components.

Test plan

How are these changes tested?

Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?

github-actions · 2024-07-01T16:11:13Z

Ishiihara · 2024-07-01T16:11:14Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @Ishiihara and the rest of your teammates on Graphite

rust/worker/src/blockstore/arrow/provider.rs

rust/worker/chroma_config.yaml

rust/worker/src/cache/cache.rs

rust/worker/src/cache/mod.rs

rust/worker/src/blockstore/arrow/block/delta.rs

HammadB · 2024-07-12T03:14:03Z

rust/worker/src/blockstore/arrow/block/delta.rs

+        for i in 0..n {
+            let key = format!("key{}", i);
+            let read = block.get::<&str, &str>("prefix", &key);
+            values_before_flush.push(read.unwrap().to_string());


nit - would prefer if the test here collected the values we generate instead of reading them from the block. avoid bugs where we persist the value wrong but its consistent

rust/worker/src/blockstore/arrow/block/delta.rs

rust/worker/src/blockstore/arrow/block/mod.rs

rust/worker/src/blockstore/arrow/blockfile.rs

rust/worker/src/blockstore/arrow/config.rs

rust/worker/src/blockstore/arrow/mod.rs

rust/worker/src/blockstore/arrow/provider.rs

rust/worker/src/blockstore/arrow/sparse_index.rs

rust/worker/src/blockstore/mod.rs

HammadB · 2024-07-12T03:30:53Z

rust/worker/src/config.rs

@@ -573,6 +629,14 @@ mod tests {
                    blockfile_provider:
                        Arrow:
                            max_block_size_bytes: 16384
+                            block_manager_config:


note - we will have to remember to update the config in staging when we merge this otherwise staging will crash

rust/worker/src/execution/operators/merge_metadata_results.rs

HammadB · 2024-07-12T03:32:47Z

rust/worker/src/execution/operators/metadata_filtering.rs

+        str::FromStr,
+        sync::{atomic::AtomicU32, Arc},
+    };
+


nit - no whitespace between use

rust/worker/src/server.rs

*Summarize the changes made by this PR.* - Improvements & Bug fixes - Makes the block size configurable, leaving default the same as before. - Add Blockfile provider config, removing a todo - Runs config tests in serial, since they can race - New functionality - None really *How are these changes tested?* Existing tests, this is a non-functional change - [x] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust None

*Summarize the changes made by this PR.* - Improvements & Bug fixes - Fixes allowed_ids and disallowed_ids to also take care of updates/deletes/upserts. For e.g. if there is an update on the log that does not update the embedding and it is in the query list then today we are never going to return this record even if it is in the top k - Adds sync points to test_embeddings + increase test timeout - Adds another rule in test_embeddings for compaction - Suppresses health check warning for filtering too much - Fixes the case when trying to commit and flush an empty block (can happen due to deletes). Sparse index start key can also get changed to something that is not SparseIndexDelimiter::Start. We decided to go ahead with flushing a dummy block if blockfile becomes fully empty so that our segment abstraction is only uninitialized until the first compaction; post that it is always initialized albeit with empty block - Fixes a bug in FTS delete document where we were incorrectly panicing - Fixes a bug in record segment apply materialization where for deletes and updates we missed writing the max offset id - Updates to metadata segment were missing updating the document and were only updating the metadata - Don't return error from the metadata segment if document is supplied as None for an update - [x] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust None

*Summarize the changes made by this PR.* - Improvements & Bug fixes - Enables s3 retry by default - New functionality - None *How are these changes tested?* - [x] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust None

HammadB · 2024-07-16T07:21:38Z

rust/worker/src/blockstore/arrow/block/delta.rs

        let block = block_manager.get(&delta.id).await.unwrap();
+        // TODO: enable this assertion after the sizing is fixed


this TODO is not needed?

Ishiihara force-pushed the liquan_block_cache_with_eviction branch 6 times, most recently from aaab13c to b253dcc Compare July 2, 2024 16:27

Ishiihara marked this pull request as ready for review July 2, 2024 16:46

HammadB reviewed Jul 2, 2024

View reviewed changes

rust/worker/src/blockstore/arrow/provider.rs Outdated Show resolved Hide resolved

Ishiihara mentioned this pull request Jul 3, 2024

[ENH] Introduce stream abstraction and enable concurrency test for blockfile #2454

Merged

1 task

Ishiihara force-pushed the liquan_block_cache_with_eviction branch from b253dcc to ce5123c Compare July 8, 2024 21:03

Ishiihara mentioned this pull request Jul 9, 2024

[ENH] Allow cache eviction for HNSW provider #2484

Merged

1 task