refactor(test): refactor some ut for hummock #18534
Conversation
| @@ -207,20 +215,22 @@ async fn test_hummock_compaction_task() { | |||
| StaticCompactionGroupId::StateDefault.into(), | |||
| ) | |||
| .await; | |||
| commit_from_meta_node( | |||
There was a problem hiding this comment.
Thanks for cleaning this up. I just realize that there are many variants of commit epoch calls in UT and their implementations are using magic numbers.
| let epoch = test_epoch(1); | ||
| let table_key_l = gen_key_from_str(VirtualNode::from_index(1), "1"); | ||
| let table_key_r = gen_key_from_str(VirtualNode::from_index(128), "1"); | ||
| let full_key_l = FullKey::for_test(TableId::new(100), table_key_l, epoch).encode(); | ||
| let full_key_r = FullKey::for_test(TableId::new(101), table_key_r, epoch).encode(); | ||
| sst_1.sst_info.key_range.left = full_key_l.into(); | ||
| sst_1.sst_info.key_range.right = full_key_r.into(); |
There was a problem hiding this comment.
I think the reason why we modify the key range here is because gen_local_sstable_info (more accurately gen_sstable_info) is filling the key range with iterator_test_key_of_epoch(1, idx, 1), which doesn't aligh with the provided table_ids. Is it possible to change the implementation of gen_sstable_info to fill in the correct key range based on table_ids? We can have a util function similar to the logic here to generate valid full key range based on provided table_ids.
There was a problem hiding this comment.
Thanks, I've fixed this logic in a new commit, so let's change it step by step. I even found some uts that relied on this wrong key_range to pass check, so I'm determined to change it .....
| assert_eq!(0, output_level_info.total_file_size); | ||
|
|
||
| // 5. get compact task | ||
| let compact_task = hummock_manager_ref |
There was a problem hiding this comment.
Why do we remove this final check?
There was a problem hiding this comment.
After review the test semantics, I decided that using the get compact task to determine whether the data was cleaned or not was not necessary, so I went from using a simpler test case
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
commit_epochfunc ofMockHumockMetaClienttable_idsforseal_and_sync_epoch(So we can then createtable_state_infoandcompaction_groupon demand via commit_epoch.)Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.