refactor(storage): refactor ReportTask event to reduce the network packet size #11711
Conversation
Codecov Report
@@ Coverage Diff @@
## main #11711 +/- ##
========================================
Coverage 70.28% 70.28%
========================================
Files 1366 1366
Lines 228417 228519 +102
========================================
+ Hits 160541 160624 +83
- Misses 67876 67895 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 12 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Li0k
changed the title
src/meta/src/hummock/manager/mod.rs
Outdated
| { | ||
| let is_trivial_reclaim = CompactStatus::is_trivial_reclaim(&compact_task); | ||
| let is_trivial_move = CompactStatus::is_trivial_move_task(&compact_task); | ||
| let can_trivial_move: bool = |
There was a problem hiding this comment.
is_trivial_reclaim, is_trivial_move and can_trivial_move are already calculated inside get_compact_task_impl.
src/meta/src/hummock/manager/mod.rs
Outdated
| let compaction = compaction_guard.deref_mut(); | ||
| let mut compact_task_assignment = | ||
| BTreeMapTransaction::new(&mut compaction.compact_task_assignment); | ||
|
|
||
| // remove task_assignment | ||
| let _ = compact_task_assignment.remove(compact_task.task_id); |
There was a problem hiding this comment.
There are two issues here:
- Why do we need to explicitly remove the task assignment on failure? Shouldn't it be done in
report_compact_task_impl? - We only new a BTreeMapTransaction and don't commit it to etcd/memory. This makes the remove a no-op
There was a problem hiding this comment.
If this task commit failed, we could still remove its assignment because we will never commit it again
src/meta/src/hummock/manager/mod.rs
Outdated
| let compaction = compaction_guard.deref_mut(); | ||
| let mut compact_task_assignment = | ||
| BTreeMapTransaction::new(&mut compaction.compact_task_assignment); | ||
|
|
||
| // remove task_assignment | ||
| let _ = compact_task_assignment.remove(compact_task.task_id); |
src/meta/src/hummock/manager/mod.rs
Outdated
| self.check_state_consistency().await; | ||
| } else { | ||
| // For trivial tasks, commit the memory state for subsequent reports. | ||
| compact_task_assignment.commit(); |
There was a problem hiding this comment.
We only commit task assignment to memory here assuming the caller will them report the task and clear the task assignment later. This logic is too subtle and is prone to error (easy to cause inconsistency between memory and etcd state). I suggest we switch to one of the following options:
a. commit the task assignment to both etcd and memory regardless of what task it is.
b. do not populate the task assignment at all for trivial task and report directly.
There was a problem hiding this comment.
We don't commit compact_status to both memory and etcd for trivial task. Will this be a problem?
There was a problem hiding this comment.
Because trivial task shall not change compact_status.
There was a problem hiding this comment.
@hzxa21 This point also confuse me.
The trivial task is added to the task assignment in order to take it out when reporting the task. Since we are no longer passing the entire compact task structure. And, if we record the trivial-task's task assignment in etcd, this will take even more time.
Signed-off-by: Little-Wallace <[email protected]>
Co-authored-by: Eric Fu <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: TennyZhuang <[email protected]>
Signed-off-by: Richard Chien <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
… match the watermark column type (#11874)
Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: TennyZhuang <[email protected]>
Signed-off-by: TennyZhuang <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Runji Wang <[email protected]>
Signed-off-by: MrCroxx <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Bugen Zhao <[email protected]>
#12279) Signed-off-by: Bugen Zhao <[email protected]> Co-authored-by: xxchan <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]> Co-authored-by: Bugen Zhao <[email protected]>
Signed-off-by: MrCroxx <[email protected]>
Signed-off-by: Runji Wang <[email protected]> Co-authored-by: Runji Wang <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: MrCroxx <[email protected]>
…nto li0k/storage_reduce_report_event_size
…nto li0k/storage_reduce_report_event_size
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Due to the upgrade of tonic, in some scenarios we found that the size of the ReportTask Event exceeded 4m.
The reason is that the CompactTask carries KeyRange information, so the size of the CompactTask will change depending on the key size of the business.
Since the original compact task is already stored in the assignment, the ReportTask doesn't need to pass back the entire compact task, just the sorted_output_ssts are sufficient. This pr is modified from the above:
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.