feat(storage): support commit multi epoch for new compaction group #17749

Li0k · 2024-07-18T14:12:11Z

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

What's changed and what's your intention?

In #17735 when we support partial checkpoint based backfill, the upstream won't wait for barrier collection of the creating downstream. Meanwhile, the creating downstream won't commit any data before completed and merged to upstream. Instead, the sst info of the creating downstream will be stored in memory. When completed and merged to upstream, all data of the creating downstream will be added together in a single commit_epoch. We will add the data to a new compaction group, and each epoch will still have a individual sub-level, with epoch as the sub-level id.

For example, let's say we start creating a new mview at epoch1. While the creating downstream is consuming the upstream mview table data at the snapshot of epoch1, the upstream will continue working and consuming data from subsequent epoch 2, 3, 4 and so on.

commit_epoch of epoch4
HummockVersion
Default Compaction Group
epoch1: epoch1 data of only upstream,
epoch2: epoch2 data of only upstream
epoch3: epoch3 data of only upstream
epoch4: epoch4 data of only upstream

When the downstream finish consuming the snapshot, it will continue consuming the epoch 2, 3, 4... data from the L0 log store. The data of the creating downstream won't be committed to hummock version until it catches up with the upstream, let's at epoch 6. When it catches up, we will complete it and merge it to the upstream graph. In epoch 6, the barrier will be injected and collected together from both upstream and downstream, and therefore the data of upstream and downstream will be mixed together. When we do commit_epoch at epoch6, we will first create a new compaction group, and add the data of the downstream mview epoch by epoch (epoch 1, 2, 3, 4, 5). Since data in epoch6 are synced together, the ssts of epoch6 will be split, some added to the original groups, and some aded to the new group.

commit_epoch of epoch6
HummockVersion
Default Compaction Group
epoch1: epoch1 data of only upstream,
epoch2: epoch2 data of only upstream
epoch3: epoch3 data of only upstream
epoch4: epoch4 data of only upstream
epoch5: epoch5 data of only upstream
epoch6: epoch6 split data of both upstream and downstream
New Compaction Group
epoch1: epoch1 data of only downstream,
epoch2: epoch2 data of only downstream
epoch3: epoch3 data of only downstream
epoch4: epoch4 data of only downstream
epoch5: epoch5 data of only downstream
epoch6: epoch6 split data of both upstream and downstream

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.

…nto li0k/storage_commit_multi_epoch

hzxa21 · 2024-07-31T06:19:31Z

src/meta/src/hummock/manager/commit_epoch.rs

@@ -161,16 +188,19 @@ impl HummockManager {
            self.env.notification_manager(),
            &self.metrics,
        );
+        let mut compaction_group_manager =
+            compaction_group_manager_guard.start_compaction_groups_txn();


Can we only acquire the compaction_group_manager lock only when batch_commit_for_new_cg is not empty in L222?

Resolved.

The original BTreeMapTransaction cannot fulfill this, because it holds the mutation reference to the RwLockGuard, and they cannot be created and stored together, and otherwise there will be the problem of self-referencing.

In this PR, I change to let BTreeMapTransaction support holding any pointer type that impl DerefMut<BTreeMap>, so that the ownership of RwLockGuard can be held by BTreeMapTransaction and there won't be self-referencing. We further introduce a new struct DerefMutForward, which forward &mut CompactionGroupManager to &mut BTreeMap<...>. This is required because RwLockGuard wraps CompactionGroupManager, but we actually need impl DerefMut<BTreeMap<...>>.

src/meta/src/hummock/manager/commit_epoch.rs

zwang28 · 2024-07-31T06:45:04Z

src/meta/src/hummock/manager/commit_epoch.rs

+
+                batch_commit_info.insert(new_compaction_group_id, epoch_to_ssts);
+            }
+            let start_sst_id = next_sstable_object_id(&self.env, new_id_count).await?;


I think we don't need to reserve new sst ids for the new compaction group, because its parent group will be StaticCompactionGroupId::NewCompactionGroup so that the reserved new sst ids won't never used anyway.

In this commit_epoch, we will insert data to the newly created compaction together in a single version delta, so we may need to reserve some sst ids for the newly added data?

The newly added SSTs in commit_epoch already have their deterministic SST ids.
We only need reserve SST ids in advance during splitting compaction group, where branched SSTs are assigned ids in the reserved range.

See

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

Line 368 in 1c2c400

pub fn init_with_parent_group(

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

Line 1326 in 1c2c400

pub fn split_sst(sst_info: &mut SstableInfo, new_sst_id: &mut u64) -> SstableInfo {

@Li0k Could you please confirm this again?

Get it. I just check the usage of new_sst_start_id , and indeed there is no need to set it when we create an empty new compaction group. Have changed to avoid passing this new_sst_start_id value along the way.

zwang28 · 2024-07-31T06:45:58Z

src/meta/src/hummock/manager/transaction.rs

+                    group_id: compaction_group_id,
+                    parent_group_id: StaticCompactionGroupId::NewCompactionGroup
+                        as CompactionGroupId,
+                    new_sst_start_id: start_sst_id,


This specific new_sst_start_id will never be used, right?

I think here we just fill in a field required to create new compaction group. What is this field used for? If it's not used for newly created compaction group, I can change to fill a trivial value.

What is this field used for

#17749 (comment)

src/meta/src/hummock/manager/transaction.rs

zwang28 · 2024-08-02T02:44:09Z

src/meta/src/hummock/manager/transaction.rs

+                    group_id: compaction_group_id,
+                    parent_group_id: StaticCompactionGroupId::NewCompactionGroup
+                        as CompactionGroupId,
+                    new_sst_start_id: 0, // No need to set it when `NewCompactionGroup`


Could you help to add an assertion in either init_with_parent_group or split_sst, to guarantee new_sst_start_id is 0 IFF NewCompactionGroup?

Added a warning in non-debug context and an assertion in debug context.

zwang28

LGTM

Li0k added 2 commits July 18, 2024 22:11

feat(storage): support commit multi epoch for new cg

0d71708

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

b68a0b3

…nto li0k/storage_commit_multi_epoch

github-actions bot added type/feature ci/run-e2e-single-node-tests labels Jul 18, 2024

zwang28 added the ci/run-e2e-meta-backup-test label Jul 19, 2024

Li0k requested a review from wenym1 July 25, 2024 08:34

wenym1 added 6 commits July 30, 2024 16:40

Merge branch 'main' into li0k/storage_commit_multi_epoch

36f2175

Merge branch 'main' into li0k/storage_commit_multi_epoch

cdad987

refine and fix test

9c47322

add to compaction group manager

006a54c

Merge branch 'main' into li0k/storage_commit_multi_epoch

c5bcda2

remove dead code

e1b188c

wenym1 marked this pull request as ready for review July 31, 2024 05:01

wenym1 requested review from hzxa21 and zwang28 July 31, 2024 05:01

hzxa21 reviewed Jul 31, 2024

View reviewed changes

zwang28 reviewed Jul 31, 2024

View reviewed changes

wenym1 added 4 commits July 31, 2024 16:17

lazily start compaction group transaction

8666c2d

fix

044c891

Merge branch 'main' into li0k/storage_commit_multi_epoch

c2194d7

remove passing start_sst_id for new group

7f0d985

wenym1 requested review from hzxa21 and zwang28 July 31, 2024 09:27

Merge branch 'main' into li0k/storage_commit_multi_epoch

913c5d1

wenym1 self-assigned this Jul 31, 2024

Merge branch 'main' into li0k/storage_commit_multi_epoch

a7c301d

zwang28 reviewed Aug 2, 2024

View reviewed changes

zwang28 approved these changes Aug 2, 2024

View reviewed changes

fix comment

5475645

wenym1 enabled auto-merge August 2, 2024 06:12

wenym1 added this pull request to the merge queue Aug 2, 2024

Merged via the queue into main with commit 09fddac Aug 2, 2024
31 of 32 checks passed

wenym1 deleted the li0k/storage_commit_multi_epoch branch August 2, 2024 07:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage): support commit multi epoch for new compaction group #17749

feat(storage): support commit multi epoch for new compaction group #17749

Li0k commented Jul 18, 2024 •

edited by wenym1

Loading

hzxa21 Jul 31, 2024

wenym1 Jul 31, 2024

zwang28 Jul 31, 2024

wenym1 Jul 31, 2024

zwang28 Jul 31, 2024 •

edited

Loading

wenym1 Jul 31, 2024

zwang28 Jul 31, 2024

wenym1 Jul 31, 2024

zwang28 Jul 31, 2024

zwang28 Aug 2, 2024

wenym1 Aug 2, 2024

zwang28 left a comment

feat(storage): support commit multi epoch for new compaction group #17749

feat(storage): support commit multi epoch for new compaction group #17749

Conversation

Li0k commented Jul 18, 2024 • edited by wenym1 Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

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

zwang28 Jul 31, 2024 • edited Loading

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

zwang28 left a comment

Choose a reason for hiding this comment

Li0k commented Jul 18, 2024 •

edited by wenym1

Loading

zwang28 Jul 31, 2024 •

edited

Loading