feat(storage): store compaction group id in per table info #17125

wenym1 · 2024-06-05T07:21:16Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

After we introduce per state table info in hummock version, we can further store the corresponding compaction group id of a table inside the per table info. Member table ids in each Levels compaction group and related fields in version delta will be deprecated, so that we don't have to maintain the consistency between table info map and compaction group member table ids.

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

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.

…mmock-snapshot-group

…roup-in-table-info

hzxa21

@Li0k @Little-Wallace PTAL

hzxa21 · 2024-06-06T03:45:33Z

proto/hummock.proto

+  option deprecated = true;
+  repeated uint32 table_ids_add = 1 [deprecated = true];
+  repeated uint32 table_ids_remove = 2 [deprecated = true];


Does it mean that we can completely deprecate GroupMetaChange in GroupDelta?

Yes, because we won't update the member_table_ids anymore.

Do we need to add deprecated in L107 then?

Added deprecated.

I think we miss [deprecated = true] in L107 for GroupMetaChange.

hzxa21 · 2024-06-06T05:16:26Z

src/storage/hummock_sdk/src/compaction_group/hummock_version_ext.rs

                );
            } else if let Some(group_change) = &summary.group_table_change {
+                // TODO: may deprecate this branch? This enum variant is not created anywhere


indeed. @Li0k@Little-Wallace What do you think? DO we still need to keep this branch for future usage?

hzxa21 · 2024-06-06T05:28:11Z

src/storage/hummock_sdk/src/compaction_group/hummock_version_ext.rs

@@ -894,6 +927,7 @@ pub fn build_initial_compaction_group_levels(
            vnode_partition_count: 0,
        });
    }
+    #[expect(deprecated)] // for backward-compatibility of previous hummock version delta


Are we planning to deprecate the whole Levels struct? I thought only member_table_ids will be deprecated.

Yes, only the field is deprecated. The annotation is applied over the whole struct because we can't apply on a single fie.d

hzxa21 · 2024-06-13T06:43:40Z

proto/hummock.proto

+  option deprecated = true;
+  repeated uint32 table_ids_add = 1 [deprecated = true];
+  repeated uint32 table_ids_remove = 2 [deprecated = true];


Do we need to add deprecated in L107 then?

hzxa21 · 2024-06-13T07:08:00Z

src/meta/src/hummock/manager/compaction_group_manager.rs

@@ -395,7 +378,12 @@ impl HummockManager {
            let group = CompactionGroupInfo {
                id: levels.group_id,
                parent_id: levels.parent_group_id,
-                member_table_ids: levels.member_table_ids.clone(),
+                member_table_ids: current_version


minor: how about sorting it before returning to keep the output consistent with what we did prior to this PR

Given that we may somehow depend on that member_table_ids is sorted, I have switched to use BtreeSet instead of HashSet so that we don't need to explicitly sort the collected vector.

hzxa21 · 2024-06-13T07:10:00Z

src/meta/src/hummock/manager/versioning.rs

@@ -337,7 +337,12 @@ pub(super) fn calc_new_write_limits(
            new_write_limits.insert(
                *id,
                WriteLimit {
-                    table_ids: levels.member_table_ids.clone(),
+                    table_ids: version


ditto: how about sorting the table_ids here?

Changed to btreeset already.

hzxa21 · 2024-06-13T07:19:50Z

src/storage/hummock_sdk/src/version.rs

+    compaction_group_member_tables: Arc<HashMap<CompactionGroupId, HashSet<TableId>>>,
+    table_compaction_group_id: Arc<HashMap<TableId, CompactionGroupId>>,


Why do we use Arc here? I don't see any caller of fn compaction_group_member_tables or table_compaction_group_id holding the Arc. Using Arc means we need to do a full in memory index rebuild instead of maintain the index incrementally on apply_delta.

Indeed. I have remove maintaining table_compaction_group_id and change to rebuild it when used, which is the same as before this PR.

For compaction_group_member_tables , I have removed the Arc around it and changed to incrementally maintain it. A debug_assert_eq is applied to compare with the fully rebuilt version to ensure correctness.

wenym1

I have addressed the comment. @hzxa21 PTAL

wenym1 · 2024-06-13T08:24:37Z

proto/hummock.proto

+  option deprecated = true;
+  repeated uint32 table_ids_add = 1 [deprecated = true];
+  repeated uint32 table_ids_remove = 2 [deprecated = true];


Added deprecated.

wenym1 · 2024-06-13T08:28:41Z

src/meta/src/hummock/manager/compaction_group_manager.rs

@@ -395,7 +378,12 @@ impl HummockManager {
            let group = CompactionGroupInfo {
                id: levels.group_id,
                parent_id: levels.parent_group_id,
-                member_table_ids: levels.member_table_ids.clone(),
+                member_table_ids: current_version


Given that we may somehow depend on that member_table_ids is sorted, I have switched to use BtreeSet instead of HashSet so that we don't need to explicitly sort the collected vector.

wenym1 · 2024-06-13T08:28:54Z

src/meta/src/hummock/manager/versioning.rs

@@ -337,7 +337,12 @@ pub(super) fn calc_new_write_limits(
            new_write_limits.insert(
                *id,
                WriteLimit {
-                    table_ids: levels.member_table_ids.clone(),
+                    table_ids: version


Changed to btreeset already.

wenym1 · 2024-06-13T08:30:55Z

src/storage/hummock_sdk/src/version.rs

+    compaction_group_member_tables: Arc<HashMap<CompactionGroupId, HashSet<TableId>>>,
+    table_compaction_group_id: Arc<HashMap<TableId, CompactionGroupId>>,


Indeed. I have remove maintaining table_compaction_group_id and change to rebuild it when used, which is the same as before this PR.

For compaction_group_member_tables , I have removed the Arc around it and changed to incrementally maintain it. A debug_assert_eq is applied to compare with the fully rebuilt version to ensure correctness.

hzxa21

LGTM. Thanks for the PR

wenym1 added 30 commits March 21, 2024 23:42

feat(storage): register member table id at commit epoch

f587d0d

fix

c802026

fix compile

1216fcf

Merge branch 'main' into yiming/register-hummock-table-in-commit

ffa06a4

unregister table id in barrier command

c1188df

fix compile

c091e59

feat(storage): support different snapshot for streaming jobs

5ea467e

unregister member table id at commit_epoch

5d291f0

impl apply_version_delta

b84b211

fill backward compatibility snapshot group

80ee13e

Merge branch 'yiming/register-hummock-table-in-commit' into yiming/hu…

ad6fa20

…mmock-snapshot-group

register snapshot group in commit epoch

f1b47b2

impl purge

4025d64

fix

0e50f63

fix compile

e0b0697

fix

a493db6

Merge branch 'main' into yiming/register-hummock-table-in-commit

a03260f

Merge branch 'yiming/register-hummock-table-in-commit' into yiming/hu…

f89ecd1

…mmock-snapshot-group

fix comment

dbdd475

Merge branch 'yiming/register-hummock-table-in-commit' into yiming/hu…

8318f5f

…mmock-snapshot-group

rename

beb78a5

fix compile

af38f68

impl system table

778080b

no extra info in create command

5860e14

unregister table id separately

f4dd843

Merge branch 'yiming/register-hummock-table-in-commit' into yiming/hu…

f934ba8

…mmock-snapshot-group

refine

a8bf79d

Merge branch 'main' into yiming/register-hummock-table-in-commit

e94774c

Merge branch 'yiming/register-hummock-table-in-commit' into yiming/hu…

7cdca7b

…mmock-snapshot-group

add more panic log

2a41e01

wenym1 added 3 commits June 5, 2024 15:54

Merge branch 'main' into yiming/hummock-snapshot-group

fc60078

remove log

7c97d95

fix compile

b717e7c

Base automatically changed from yiming/hummock-snapshot-group to main June 5, 2024 11:50

wenym1 added 3 commits June 5, 2024 23:25

Merge branch 'main' into yiming/hummock-snapshot-group

c05e376

Merge branch 'yiming/hummock-snapshot-group' into yiming/compaction-g…

3ad89de

…roup-in-table-info

fix test

8030edc

wenym1 marked this pull request as ready for review June 5, 2024 15:34

hzxa21 reviewed Jun 6, 2024

View reviewed changes

Merge branch 'main' into yiming/compaction-group-in-table-info

a98bf38

github-actions bot added the breaking-change label Jun 6, 2024

wenym1 requested review from hzxa21, Li0k, Little-Wallace and zwang28 June 6, 2024 07:14

wenym1 added 2 commits June 12, 2024 18:01

Merge branch 'main' into yiming/compaction-group-in-table-info

ef81602

remove unnecessary logic

0ffc3ad

hzxa21 reviewed Jun 13, 2024

View reviewed changes

wenym1 added 2 commits June 13, 2024 15:25

Merge branch 'main' into yiming/compaction-group-in-table-info

0f55e99

fix comment

9e53e9a

wenym1 commented Jun 13, 2024

View reviewed changes

Merge branch 'main' into yiming/compaction-group-in-table-info

ae1b046

hzxa21 approved these changes Jun 13, 2024

View reviewed changes

wenym1 added this pull request to the merge queue Jun 13, 2024

Merged via the queue into main with commit cccf946 Jun 13, 2024
32 of 33 checks passed

wenym1 deleted the yiming/compaction-group-in-table-info branch June 13, 2024 10:14

zwang28 mentioned this pull request Jun 14, 2024

fix(test): fix regex #17249

Merged

9 tasks

wenym1 mentioned this pull request Sep 12, 2024

Tracking: support partial checkpoint #14041

Open

22 tasks

Li0k mentioned this pull request Sep 18, 2024

refactor(storage): remove legacy delta type GroupMetaChange and GroupTableChange #18585

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage): store compaction group id in per table info #17125

feat(storage): store compaction group id in per table info #17125

wenym1 commented Jun 5, 2024 •

edited

Loading

hzxa21 left a comment

hzxa21 Jun 6, 2024

wenym1 Jun 6, 2024

hzxa21 Jun 13, 2024

wenym1 Jun 13, 2024

hzxa21 Jun 13, 2024

hzxa21 Jun 6, 2024

hzxa21 Jun 6, 2024

wenym1 Jun 6, 2024

hzxa21 Jun 13, 2024

hzxa21 Jun 13, 2024

wenym1 Jun 13, 2024

hzxa21 Jun 13, 2024

wenym1 Jun 13, 2024

hzxa21 Jun 13, 2024

wenym1 Jun 13, 2024

wenym1 left a comment

wenym1 Jun 13, 2024

wenym1 Jun 13, 2024

wenym1 Jun 13, 2024

wenym1 Jun 13, 2024

hzxa21 left a comment

		compaction_group_member_tables: Arc<HashMap<CompactionGroupId, HashSet<TableId>>>,
		table_compaction_group_id: Arc<HashMap<TableId, CompactionGroupId>>,

feat(storage): store compaction group id in per table info #17125

feat(storage): store compaction group id in per table info #17125

Conversation

wenym1 commented Jun 5, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

hzxa21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenym1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxa21 left a comment

Choose a reason for hiding this comment

wenym1 commented Jun 5, 2024 •

edited

Loading