feat: Add compression capabilities for StreamNode in PbTableFragments. #13598
Conversation
shanicky
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #13598 +/- ##
==========================================
- Coverage 68.23% 68.20% -0.03%
==========================================
Files 1525 1526 +1
Lines 262188 262319 +131
==========================================
+ Hits 178899 178920 +21
- Misses 83289 83399 +110
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
shanicky
force-pushed
the
peng/compress-table-fragments
branch
from
5e95712 to
b1ab4f3
Compare
src/meta/src/controller/fragment.rs
Outdated
| let mut table_fragments = table_fragments; | ||
|
|
||
| if table_fragments.graph_render_type == GraphRenderType::RenderTemplate as i32 { | ||
| downgrade_table_fragments(&mut table_fragments); |
There was a problem hiding this comment.
We'd better support transform between sql model and templated kv directly for better performance. Not urgent and can be done in future PR.
|
|
||
| use crate::util::stream_graph_visitor::visit_stream_node; | ||
|
|
||
| pub fn downgrade_table_fragments(table_fragments: &mut PbTableFragments) { |
There was a problem hiding this comment.
Would you mind adding docs for these functions?
| pub fn downgrade_table_fragments(table_fragments: &mut PbTableFragments) { | ||
| assert_eq!( | ||
| table_fragments.graph_render_type, | ||
| GraphRenderType::RenderTemplate as i32 | ||
| ); |
There was a problem hiding this comment.
| pub fn downgrade_table_fragments(table_fragments: &mut PbTableFragments) { | |
| assert_eq!( | |
| table_fragments.graph_render_type, | |
| GraphRenderType::RenderTemplate as i32 | |
| ); | |
| #[easy_ext::ext(TableFragmentsRenderCompat)] | |
| impl TableFragments { | |
| pub fn ensure_downgraded(&mut self) { | |
| if let Unspecified = table_fragments.graph_render_type { | |
| return; | |
| } |
There was a problem hiding this comment.
Indeed, easy_ext is quite useful. However, I believe that the calls here should be explicit and within control. Hence, I would prefer to directly assert fail when there are repeated calls to compress/uncompress. 🤔
There was a problem hiding this comment.
Then ensure sounds not that appropriate. 😢
|
By the way, I'm considering if it's worth trying the approach proposed here, which could be more transparent to the application. |
This should be good, if it can be achieved through middleware, it should be able to be done very transparently. |
… update graph_render_type.
…ns. Updated comments and renamed `downgrade_table_fragments` to `uncompress_table_fragments`.
…compressed/uncompressed table fragments before passing to function.
shanicky
force-pushed
the
peng/compress-table-fragments
branch
from
9f6dee2 to
266890e
Compare
|
I added two conversions here. The code with these conversions has passed all the CI tests, so it seems to be fine. |
…to `compress` in multiple files.
shanicky
changed the title
|
This PR has been open for 60 days with no activity. Could you please update the status? Feel free to ping a reviewer if you are waiting for review. |
The results in #15558 show that the simple gzip compression will work pretty well. Can we take a try? Or have we postponed this PR due to upcoming migration to SQL meta store backend? 😄 |
|
👀 |
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
This PR attempts to compress duplicate node bodies in PbTableFragments::fragment::actor::nodes. The aim is to possibly reduce the RPC and meta storage to avoid ‘request limit’ errors.
before
after
Checklist
./risedev check(or alias,./risedev c)