storage: do not fetch all tables during every query.

[Little-Wallace]

Signed-off-by: Little-Wallace [email protected]

What's changed and what's your intention?

close #1507
Although the meta most of tables will be cached, if there are too many files in the whole cluster , the bloom-filter-data would cost a lot of memory. In fact, most of request only concerned about a few data in a small range of the current executor , so they do not need to fetch all table from the table-cache.

Checklist

• I have written necessary docs and comments
• I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

[Little-Wallace]

I will add some unit test to ensure correctness and add more benchmark for iterator test.

[StrikeW]

Better to add some comments in the code😊

[BugenZhao]

No need to collect here?

[Little-Wallace]

I do not understand ....

[soundOfDestiny]

Are get_compact_task and report_compact_tasks calls here proper? better ask @zwang28

[soundOfDestiny]

why does first level have to be Overlapping?

[soundOfDestiny]

What if compact task failed?

[codecov]

Codecov Report

Merging #1558 (1e97104) into main (fbaad8f) will decrease coverage by 0.06%.
The diff coverage is 56.02%.

@@             Coverage Diff              @@
##               main    #1558      +/-   ##
============================================
- Coverage     69.91%   69.85%   -0.07%     
  Complexity     2766     2766              
============================================
  Files          1052     1052              
  Lines         92464    92678     +214     
  Branches       1790     1790              
============================================
+ Hits          64650    64741      +91     
- Misses        26923    27046     +123     
  Partials        891      891              

Flag	Coverage Δ
java	61.01% <ø> (ø)
rust	71.68% <56.02%> (-0.09%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/bench/ss_bench/main.rs	0.81% <0.00%> (-0.12%)	⬇️
src/bench/ss_bench/operations/mod.rs	0.00% <0.00%> (ø)
src/bench/ss_bench/operations/write_batch.rs	0.00% <0.00%> (ø)
src/meta/src/hummock/mock_hummock_meta_client.rs	37.50% <0.00%> (-13.23%)	⬇️
src/meta/src/hummock/mod.rs	30.27% <0.00%> (-2.40%)	⬇️
src/meta/src/hummock/compaction.rs	77.28% <41.37%> (-2.61%)	⬇️
src/storage/src/hummock/utils.rs	87.80% <63.63%> (-8.87%)	⬇️
src/storage/src/hummock/local_version_manager.rs	85.76% <66.66%> (+1.17%)	⬆️
src/storage/src/hummock/mod.rs	72.40% <68.90%> (-1.27%)	⬇️
src/storage/src/hummock/snapshot_tests.rs	96.69% <91.66%> (-0.68%)	⬇️
... and 28 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[Little-Wallace]

I run a benchmark to test the filter effect for latency.

benchmark command

 cargo run --release --bin ss-bench -- \
 --benchmarks "writebatch,prefixscanrandom"  --batch-size 100000 \
 --writes 500000 \
 --reads 0 \
 --scans 10000 \
 --deletes 0 \
 --concurrency-num 1 \
 --seed 233 \
 --statistics --store "hummock+minio://minioadmin:[email protected]:9000/bucket" --compact-level-after-write 1 --table-size-mb=2

benchmark result

current PR
prefixscanrandom
latency:
min: 8.416 micro sec,
mean: 13.158 micro sec,
p50: 9.541 micro sec,
p90: 18.250 micro sec,
p99: 24.208 micro sec,
max: 797.125 micro sec,
std_dev: 21845.084;
QPS: 0 1772720 bytes/sec

main
prefixscanrandom
latency:
min: 30.208 micro sec,
mean: 38.120 micro sec,
p50: 33.083 micro sec,
p90: 47.708 micro sec,
p99: 73.958 micro sec,
max: 11.422 milli sec,
std_dev: 114168.059;
QPS: 0 623231 bytes/sec

Because the main branch can not run a iterator bench after compaction, I commented out some code from the current branch to replace the main branch.

            let mut overlapped_sstable_iters = vec![];
            for level in &version.levels() {
                let table_ids = level
                    .table_infos
                    .iter()
                    // .filter(|info| {
                    //     let table_range = info.key_range.as_ref().unwrap();
                    //     let table_start = user_key(table_range.left.as_slice());
                    //     let table_end = user_key(table_range.right.as_slice());
                    //     range_overlap(&key_range, table_start, table_end, false)
                    // })
                    .map(|info| info.id)
                    .collect_vec();
                if table_ids.is_empty() {
                    continue;
                }
                table_count += table_ids.len();
                let tables = self
                    .local_version_manager
                    .pick_few_tables(&table_ids)
                    .await?;
                // match level.level_type() {
                //     LevelType::Overlapping => {
                        for table in tables.into_iter().rev() {
                            overlapped_sstable_iters.push(Box::new(SSTableIterator::new(
                                table,
                                self.sstable_store.clone(),
                            ))
                                as BoxedHummockIterator);
                        }
                //     }
                //     LevelType::Nonoverlapping => overlapped_sstable_iters.push(Box::new(
                //         ConcatIterator::new(tables, self.sstable_store.clone()),
                //     )),
                // }
            }

[soundOfDestiny]

lgtm

[zwang28]

There is a get_compact_task method in HummockManager, can it be used directly?

[Little-Wallace]

Yes. But there is a difficult that HummockManager trigger it automatic but i need a manual interface

[zwang28]

Yes. But there is a difficult that HummockManager trigger it automatic but i need a manual interface

HummockManager::get_compact_task itself is manual and we do have a reference to hummock_manager in MockHummockMetaClient.
The automatic compact trigger will only be started along with meta node, which is not the case here.

[Little-Wallace]

ok, I will remove it in #1694. please take a look