feat(storage): introduce non-proto type for vnode bitmap #3030
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
license-eye has totally checked 827 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 824 | 2 | 1 | 0 |
Click to see the invalid file list
- src/common/src/consistent_hash/mod.rs
- src/common/src/consistent_hash/vnode.rs
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## main #3030 +/- ##
=======================================
Coverage 73.26% 73.26%
=======================================
Files 730 731 +1
Lines 98392 98441 +49
=======================================
+ Hits 72082 72122 +40
- Misses 26310 26319 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
| if let Ok(pos) = | ||
| bitmaps.binary_search_by_key(&self.table_id, |bitmap| bitmap.get_table_id()) | ||
| { | ||
| let text = &bitmaps[pos]; |
There was a problem hiding this comment.
a small question: why it's called "text"? 👀
There was a problem hiding this comment.
Just copied the function from src/storage/src/hummock/utils.rs. 🤣 Weird indeed. Lemme change it.
| // table owns. | ||
| #[derive(Clone, Default)] | ||
| pub struct VNodeBitmap { | ||
| table_id: u32, |
There was a problem hiding this comment.
Looks a little strange. Are you sure every VNodeBitmap needs a table_id? The most intuitive representation may be only bitmap inside (e.g. Vec<u8>) of it, while the table_id is out of it (e.g. HashMap<u32, VNodeBitmap > )
There was a problem hiding this comment.
There will be a VNodeBitmap in every Keyspace of a state table, so table_id is in need here. Also, VNodeBitmap will be added to the interface of StateStore, where we use it for read pruning. In this case, table_id is also necessary. Any other case to use VNodeBitmap?
There was a problem hiding this comment.
And from the aspect of logic, the struct is designed for read pruning, which depends on state table. So having a table_id is reasonable, right? 🤔
What's changed and what's your intention?
Summarize your change
Introduce non-proto type for vnode bitmap.
Checklist
./risedev check(or alias,./risedev c)Refer to a related PR or issue link (optional)
#2882 should depend on this PR. More specifically, the non-proto type introduced by this PR will be used in keyspace and interfaces of state store (get, scan, etc.).