Skip to content

Commit 1246a8b

Browse files
zwang28zwang28
authored
refactor(meta): store compaction group info in HummockVersion (#7817)
#6246 has made update of compaction group info(e.g. member tables) and hummock version info in sync by using one transaction. But due to the fact that compaction group info and hummock version info are managed in different places (`CompactionGroupManagerInner` and `HummockManager`), the code to update them has been hard to read/maintain to some extent. For example, - `CompactionGroupManagerInner` is a member of `HummockManager`, but its methods [take ref of `HummockManager`](https://github.com/risingwavelabs/risingwave/blob/037f51ae7c9608e68d8697f7f87b0e188a78f161/src/meta/src/hummock/manager/compaction_group_manager.rs#L350). - [`sync_group method`](https://github.com/risingwavelabs/risingwave/blob/037f51ae7c9608e68d8697f7f87b0e188a78f161/src/meta/src/hummock/manager/mod.rs#L1469)'s parameters `compaction`,`versioning`,`compaction_groups` actually implies 3 corresponding locks have been taken. An unconscious locking request inside the method will cause dead lock. - [`sync_group`] takes a `trx_extern_part`. The external caller of `sync_group` must remember to check `trx_extern_part` after `sync_group` returns, in order to make sure the external trx is committed, if it has not been done by `sync_group`. - The lock order of `CompactionGroupManagerInner` and `versioning` need be maintained carefully. This PR wants to simplify the code, by - store compaction group info(except compaction config) in hummock version directly. We remove the universal [`sync_group`], and split its functionality into `register_table_ids`/`unregister_table_ids`. Note that because we haven't supported compaction group split API, some functionality of [`sync_group`] may not be kept. For example [`sync_group`] handles SST split but current codebase never trigger it when calling [`sync_group`] . - make `CompactionGroupManager` only responsible for compaction config maintenance now, which doesn't need to be in sync with hummock version. Approved-By: soundOfDestiny Approved-By: Li0k Co-Authored-By: zwang28 <[email protected]> Co-Authored-By: zwang28 <[email protected]>
1 parent 89ff6d9 commit 1246a8b

40 files changed

+972
-1429
lines changed

‎dashboard/proto/gen/hummock.ts

Lines changed: 126 additions & 131 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎proto/hummock.proto

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,12 @@ message GroupConstruct {
5858
// If `parent_group_id` is not 0, it means `parent_group_id` splits into `parent_group_id` and this group, so this group is not empty initially.
5959
uint64 parent_group_id = 2;
6060
repeated uint32 table_ids = 3;
61+
uint64 group_id = 4;
62+
}
63+
64+
message GroupMetaChange {
65+
repeated uint32 table_ids_add = 1;
66+
repeated uint32 table_ids_remove = 2;
6167
}
6268

6369
message GroupDestroy {}
@@ -67,6 +73,7 @@ message GroupDelta {
6773
IntraLevelDelta intra_level = 1;
6874
GroupConstruct group_construct = 2;
6975
GroupDestroy group_destroy = 3;
76+
GroupMetaChange group_meta_change = 4;
7077
}
7178
}
7279

@@ -79,6 +86,9 @@ message HummockVersion {
7986
message Levels {
8087
repeated Level levels = 1;
8188
OverlappingLevel l0 = 2;
89+
uint64 group_id = 3;
90+
uint64 parent_group_id = 4;
91+
repeated uint32 member_table_ids = 5;
8292
}
8393
uint64 id = 1;
8494
// Levels of each compaction group
@@ -274,12 +284,19 @@ message CompactStatus {
274284
repeated LevelHandler level_handlers = 2;
275285
}
276286

287+
// Config info of compaction group.
277288
message CompactionGroup {
289+
uint64 id = 1;
290+
CompactionConfig compaction_config = 4;
291+
}
292+
293+
// Complete info of compaction group.
294+
// The info is the aggregate of `HummockVersion` and `CompactionGroupConfig`
295+
message CompactionGroupInfo {
278296
uint64 id = 1;
279297
uint64 parent_id = 2;
280298
repeated uint32 member_table_ids = 3;
281299
CompactionConfig compaction_config = 4;
282-
map<uint32, TableOption> table_id_to_options = 5;
283300
}
284301

285302
message CompactTaskAssignment {
@@ -389,13 +406,6 @@ message ReportVacuumTaskResponse {
389406
common.Status status = 1;
390407
}
391408

392-
message GetCompactionGroupsRequest {}
393-
394-
message GetCompactionGroupsResponse {
395-
common.Status status = 1;
396-
repeated CompactionGroup compaction_groups = 2;
397-
}
398-
399409
message TriggerManualCompactionRequest {
400410
uint64 compaction_group_id = 1;
401411
KeyRange key_range = 2;
@@ -458,7 +468,7 @@ message RiseCtlGetPinnedSnapshotsSummaryResponse {
458468

459469
message InitMetadataForReplayRequest {
460470
repeated catalog.Table tables = 1;
461-
repeated CompactionGroup compaction_groups = 2;
471+
repeated CompactionGroupInfo compaction_groups = 2;
462472
}
463473

464474
message InitMetadataForReplayResponse {}
@@ -489,7 +499,7 @@ message RiseCtlListCompactionGroupRequest {}
489499

490500
message RiseCtlListCompactionGroupResponse {
491501
common.Status status = 1;
492-
repeated CompactionGroup compaction_groups = 2;
502+
repeated CompactionGroupInfo compaction_groups = 2;
493503
}
494504

495505
message RiseCtlUpdateCompactionConfigRequest {
@@ -546,7 +556,6 @@ service HummockManagerService {
546556
rpc GetNewSstIds(GetNewSstIdsRequest) returns (GetNewSstIdsResponse);
547557
rpc SubscribeCompactTasks(SubscribeCompactTasksRequest) returns (stream SubscribeCompactTasksResponse);
548558
rpc ReportVacuumTask(ReportVacuumTaskRequest) returns (ReportVacuumTaskResponse);
549-
rpc GetCompactionGroups(GetCompactionGroupsRequest) returns (GetCompactionGroupsResponse);
550559
rpc TriggerManualCompaction(TriggerManualCompactionRequest) returns (TriggerManualCompactionResponse);
551560
rpc ReportFullScanTask(ReportFullScanTaskRequest) returns (ReportFullScanTaskResponse);
552561
rpc TriggerFullGC(TriggerFullGCRequest) returns (TriggerFullGCResponse);

‎src/common/src/catalog/mod.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -204,8 +204,6 @@ impl From<TableId> for u32 {
204204
}
205205
}
206206

207-
// TODO: TableOption is duplicated with the properties in table catalog, We can refactor later to
208-
// directly fetch such options from catalog when creating compaction jobs.
209207
#[derive(Clone, Debug, PartialEq, Default, Copy)]
210208
pub struct TableOption {
211209
pub retention_seconds: Option<u32>, // second

‎src/meta/src/backup_restore/meta_snapshot_builder.rs

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -69,13 +69,11 @@ impl<S: MetaStore> MetaSnapshotBuilder<S> {
6969
.next()
7070
.ok_or_else(|| anyhow!("hummock version stats not found in meta store"))?;
7171
let compaction_groups =
72-
crate::hummock::compaction_group::CompactionGroup::list_at_snapshot::<S>(
73-
&meta_store_snapshot,
74-
)
75-
.await?
76-
.iter()
77-
.map(MetadataModel::to_protobuf)
78-
.collect();
72+
crate::hummock::model::CompactionGroup::list_at_snapshot::<S>(&meta_store_snapshot)
73+
.await?
74+
.iter()
75+
.map(MetadataModel::to_protobuf)
76+
.collect();
7977
let table_fragments =
8078
crate::model::TableFragments::list_at_snapshot::<S>(&meta_store_snapshot)
8179
.await?

‎src/meta/src/backup_restore/restore.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ use risingwave_common::config::MetaBackend;
2121

2222
use crate::backup_restore::utils::{get_backup_store, get_meta_store, MetaStoreBackendImpl};
2323
use crate::dispatch_meta_store;
24-
use crate::hummock::compaction_group::CompactionGroup;
24+
use crate::hummock::model::CompactionGroup;
2525
use crate::model::{MetadataModel, TableFragments};
2626
use crate::storage::{MetaStore, DEFAULT_COLUMN_FAMILY};
2727

‎src/meta/src/barrier/recovery.rs

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -88,17 +88,14 @@ where
8888

8989
// unregister compaction group for dirty table fragments.
9090
let _ = self.hummock_manager
91-
.unregister_table_ids(
92-
&to_drop_streaming_ids
93-
.iter()
94-
.map(|t| t.table_id)
95-
.collect_vec(),
91+
.unregister_table_fragments_vec(
92+
&to_drop_table_fragments
9693
)
9794
.await.inspect_err(|e|
9895
tracing::warn!(
99-
"Failed to unregister compaction group for {:#?}.\nThey will be cleaned up on node restart.\n{:#?}",
100-
to_drop_streaming_ids,
101-
e)
96+
"Failed to unregister compaction group for {:#?}. They will be cleaned up on node restart. {:#?}",
97+
to_drop_table_fragments,
98+
e)
10299
);
103100

104101
// clean up source connector dirty changes.

‎src/meta/src/hummock/compaction/level_selector.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,8 @@ use crate::hummock::compaction::{
3636
create_overlap_strategy, CompactionPicker, CompactionTask, LocalPickerStatistic,
3737
LocalSelectorStatistic, MinOverlappingPicker,
3838
};
39-
use crate::hummock::compaction_group::CompactionGroup;
4039
use crate::hummock::level_handler::LevelHandler;
40+
use crate::hummock::model::CompactionGroup;
4141
use crate::rpc::metrics::MetaMetrics;
4242

4343
const SCORE_BASE: u64 = 100;
@@ -364,7 +364,7 @@ impl LevelSelector for SpaceReclaimCompactionSelector {
364364
let dynamic_level_core = DynamicLevelSelectorCore::new(group.compaction_config.clone());
365365
let mut picker = SpaceReclaimCompactionPicker::new(
366366
group.compaction_config.max_space_reclaim_bytes,
367-
group.member_table_ids.clone(),
367+
levels.member_table_ids.iter().cloned().collect(),
368368
);
369369
let ctx = dynamic_level_core.calculate_level_base_size(levels);
370370
let state = self
@@ -636,6 +636,7 @@ pub mod tests {
636636
let mut levels = Levels {
637637
levels,
638638
l0: Some(generate_l0_nonoverlapping_sublevels(vec![])),
639+
..Default::default()
639640
};
640641
let ctx = selector.calculate_level_base_size(&levels);
641642
assert_eq!(ctx.base_level, 2);
@@ -712,6 +713,7 @@ pub mod tests {
712713
3,
713714
10,
714715
))),
716+
..Default::default()
715717
};
716718

717719
let mut selector = DynamicLevelSelector::default();

‎src/meta/src/hummock/compaction/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,8 @@ pub use crate::hummock::compaction::level_selector::{
3737
SpaceReclaimCompactionSelector, TtlCompactionSelector,
3838
};
3939
use crate::hummock::compaction::overlap_strategy::{OverlapStrategy, RangeOverlapStrategy};
40-
use crate::hummock::compaction_group::CompactionGroup;
4140
use crate::hummock::level_handler::LevelHandler;
41+
use crate::hummock::model::CompactionGroup;
4242
use crate::rpc::metrics::MetaMetrics;
4343

4444
pub struct CompactStatus {

‎src/meta/src/hummock/compaction/picker/base_level_compaction_picker.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,7 @@ pub mod tests {
251251
generate_table(0, 1, 301, 400, 1),
252252
],
253253
)],
254+
..Default::default()
254255
};
255256
let mut local_stats = LocalPickerStatistic::default();
256257
let mut levels_handler = vec![LevelHandler::new(0), LevelHandler::new(1)];
@@ -335,6 +336,7 @@ pub mod tests {
335336
sub_levels: vec![],
336337
total_file_size: 0,
337338
}),
339+
..Default::default()
338340
};
339341
push_tables_level0_nonoverlapping(&mut levels, vec![generate_table(1, 1, 50, 60, 2)]);
340342
push_tables_level0_nonoverlapping(
@@ -390,6 +392,7 @@ pub mod tests {
390392
sub_levels: vec![],
391393
total_file_size: 0,
392394
}),
395+
..Default::default()
393396
};
394397
push_tables_level0_nonoverlapping(
395398
&mut levels,
@@ -431,6 +434,7 @@ pub mod tests {
431434
l0: Some(generate_l0_nonoverlapping_sublevels(vec![generate_table(
432435
1, 1, 160, 280, 2,
433436
)])),
437+
..Default::default()
434438
};
435439

436440
let mut levels_handler = vec![LevelHandler::new(0), LevelHandler::new(1)];
@@ -487,6 +491,7 @@ pub mod tests {
487491
generate_table(1, 1, 100, 210, 2),
488492
generate_table(2, 1, 200, 250, 2),
489493
])),
494+
..Default::default()
490495
};
491496
let mut levels_handler = vec![LevelHandler::new(0), LevelHandler::new(1)];
492497

@@ -528,6 +533,7 @@ pub mod tests {
528533
sub_level_id: 0,
529534
}],
530535
l0: Some(generate_l0_nonoverlapping_sublevels(vec![])),
536+
..Default::default()
531537
};
532538
push_tables_level0_nonoverlapping(
533539
&mut levels,
@@ -582,6 +588,7 @@ pub mod tests {
582588
sub_level_id: 0,
583589
}],
584590
l0: Some(generate_l0_nonoverlapping_sublevels(vec![])),
591+
..Default::default()
585592
};
586593
push_tables_level0_nonoverlapping(
587594
&mut levels,
@@ -616,6 +623,7 @@ pub mod tests {
616623
let levels = Levels {
617624
l0: Some(l0),
618625
levels: vec![generate_level(1, vec![generate_table(3, 1, 0, 100000, 1)])],
626+
..Default::default()
619627
};
620628
let levels_handler = vec![LevelHandler::new(0), LevelHandler::new(1)];
621629

@@ -689,6 +697,7 @@ pub mod tests {
689697
let levels = Levels {
690698
l0: Some(l0),
691699
levels: vec![generate_level(1, vec![generate_table(3, 1, 0, 100000, 1)])],
700+
..Default::default()
692701
};
693702
let mut levels_handler = vec![LevelHandler::new(0), LevelHandler::new(1)];
694703
let mut local_stats = LocalPickerStatistic::default();

‎src/meta/src/hummock/compaction/picker/manual_compaction_picker.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,7 @@ pub mod tests {
333333
use crate::hummock::compaction::level_selector::{LevelSelector, ManualCompactionSelector};
334334
use crate::hummock::compaction::overlap_strategy::RangeOverlapStrategy;
335335
use crate::hummock::compaction::LocalSelectorStatistic;
336-
use crate::hummock::compaction_group::CompactionGroup;
336+
use crate::hummock::model::CompactionGroup;
337337
use crate::hummock::test_utils::iterator_test_key_of_epoch;
338338

339339
fn clean_task_state(level_handler: &mut LevelHandler) {
@@ -397,6 +397,7 @@ pub mod tests {
397397
let mut levels = Levels {
398398
levels,
399399
l0: Some(generate_l0_nonoverlapping_sublevels(vec![])),
400+
..Default::default()
400401
};
401402
let mut levels_handler = vec![
402403
LevelHandler::new(0),
@@ -582,6 +583,7 @@ pub mod tests {
582583
let levels = Levels {
583584
levels,
584585
l0: Some(l0),
586+
..Default::default()
585587
};
586588

587589
let levels_handler = vec![
@@ -609,6 +611,7 @@ pub mod tests {
609611
let levels = Levels {
610612
levels,
611613
l0: Some(l0),
614+
..Default::default()
612615
};
613616

614617
let levels_handler = vec![LevelHandler::new(0), LevelHandler::new(1)];
@@ -628,6 +631,7 @@ pub mod tests {
628631
let levels = Levels {
629632
levels,
630633
l0: Some(l0),
634+
..Default::default()
631635
};
632636
let levels_handler = vec![LevelHandler::new(0), LevelHandler::new(1)];
633637
let option = ManualCompactionOption {
@@ -1167,6 +1171,7 @@ pub mod tests {
11671171
let levels = Levels {
11681172
levels,
11691173
l0: Some(l0),
1174+
..Default::default()
11701175
};
11711176
let mut levels_handler = (0..5).map(LevelHandler::new).collect_vec();
11721177
let mut local_stats = LocalSelectorStatistic::default();
@@ -1273,6 +1278,7 @@ pub mod tests {
12731278
let levels = Levels {
12741279
levels,
12751280
l0: Some(l0),
1281+
..Default::default()
12761282
};
12771283
let mut levels_handler = (0..5).map(LevelHandler::new).collect_vec();
12781284
let mut local_stats = LocalSelectorStatistic::default();

‎src/meta/src/hummock/compaction/picker/min_overlap_compaction_picker.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,7 @@ pub mod tests {
173173
let levels = Levels {
174174
levels,
175175
l0: Some(generate_l0_nonoverlapping_sublevels(vec![])),
176+
..Default::default()
176177
};
177178
let mut level_handlers = vec![
178179
LevelHandler::new(0),
@@ -246,6 +247,7 @@ pub mod tests {
246247
let levels = Levels {
247248
levels,
248249
l0: Some(generate_l0_nonoverlapping_sublevels(vec![])),
250+
..Default::default()
249251
};
250252
let levels_handler = vec![
251253
LevelHandler::new(0),

‎src/meta/src/hummock/compaction/picker/space_reclaim_compaction_picker.rs

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ mod test {
118118
};
119119
use crate::hummock::compaction::level_selector::SpaceReclaimCompactionSelector;
120120
use crate::hummock::compaction::{LevelSelector, LocalSelectorStatistic};
121-
use crate::hummock::compaction_group::CompactionGroup;
121+
use crate::hummock::model::CompactionGroup;
122122

123123
#[test]
124124
fn test_space_reclaim_compaction_selector() {
@@ -127,7 +127,7 @@ mod test {
127127
.max_level(4)
128128
.max_space_reclaim_bytes(max_space_reclaim_bytes)
129129
.build();
130-
let mut group_config = CompactionGroup::new(1, config);
130+
let group_config = CompactionGroup::new(1, config);
131131

132132
let l0 = generate_l0_nonoverlapping_sublevels(vec![]);
133133
assert_eq!(l0.sub_levels.len(), 0);
@@ -160,9 +160,10 @@ mod test {
160160
},
161161
];
162162
assert_eq!(levels.len(), 4);
163-
let levels = Levels {
163+
let mut levels = Levels {
164164
levels,
165165
l0: Some(l0),
166+
..Default::default()
166167
};
167168
let mut levels_handler = (0..5).map(LevelHandler::new).collect_vec();
168169
let mut local_stats = LocalSelectorStatistic::default();
@@ -257,7 +258,7 @@ mod test {
257258
}
258259
}
259260

260-
group_config.member_table_ids = HashSet::from_iter([2, 3, 4, 5, 6, 7, 8, 9, 10]);
261+
levels.member_table_ids = vec![2, 3, 4, 5, 6, 7, 8, 9, 10];
261262
// pick space reclaim
262263
let task = selector.pick_compaction(
263264
1,
@@ -276,7 +277,7 @@ mod test {
276277
}
277278
}
278279

279-
group_config.member_table_ids = HashSet::from_iter([2, 3, 4, 5, 6, 7, 8, 9]);
280+
levels.member_table_ids = vec![2, 3, 4, 5, 6, 7, 8, 9];
280281
// pick space reclaim
281282
let task = selector
282283
.pick_compaction(

0 commit comments

Comments
 (0)