refactor(storage): maintain sub_level id order #18915

zwang28 · 2024-10-15T06:07:00Z

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

What's changed and what's your intention?

This PR reverts #18886, then resolves the same issue by maintaining a new field max_sub_level_id in HummockVersion.
It has advantage of keeping the non-decreasing order of sub_level_id, in contrast to #18886.

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.

This reverts commit 798896f.

hzxa21 · 2024-10-15T07:21:12Z

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

+                        .latest_version()
+                        .max_sub_level_id
+                        .map(|id| id + 1)
+                        .unwrap_or(FIRST_SUB_LEVEL_ID),


To be backward compatible, should we init the max_sub_level_id on meta startup to be the actual max sub-level id and we use the max_sub_level_id field only for the new sublevel's id?

To be backward compatible, should we init the max_sub_level_id on meta startup

This has been done in src/storage/hummock_sdk/src/version.rs

Therefore, reaching line 169 indicates that the version is indeed a fresh new one.

Alternatively we can make the in-mem HummockVersion:: max_sub_level_id u64 directly instead of Option<64>, in order to avoid such unwrap_or whenever reading max_sub_level_id.

I think it's ok for now, the newly introduced fields keep the Option restrictions.

When line 169 is reached, the sub_level will no longer exist in the compaciton group, so it is correct to use FIRST_SUB_LEVEL_ID for initialisation.

we use the max_sub_level_id field only for the new sublevel's id

True, it's a bug. Thanks.

wenym1 · 2024-10-15T07:20:53Z

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

@@ -764,6 +759,14 @@ impl HummockVersion {
            &version_delta.state_table_info_delta,
            &changed_table_info,
        );
+
+        // The max_sub_level_id may have been increased, e.g. during commit_epoch or merge_group.


We should max with self.max_sub_level_id and otherwise it will be reset to 0 when all data are clean.

Fixed. Thanks.

wenym1 · 2024-10-15T07:24:30Z

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

@@ -883,7 +886,7 @@ impl HummockVersion {
            )
        });

-        group_split::merge_levels(left_levels, right_levels);
+        group_split::merge_levels(left_levels, right_levels, self.max_sub_level_id);


Not sure if it's better to maintain a per-compaction group max_sub_level_id instead of a global one, and then we don't need to pass the global one here.

I'm open to either option. @Li0k

I prefer global uniqueness, for me at the moment this sub level id has no more semantics. So globally unique is more concise.

Li0k

Rest LGTM, thanks for the quick fix

Li0k · 2024-10-15T08:37:08Z

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

+                        .latest_version()
+                        .max_sub_level_id
+                        .map(|id| id + 1)
+                        .unwrap_or(FIRST_SUB_LEVEL_ID),


I think it's ok for now, the newly introduced fields keep the Option restrictions.

When line 169 is reached, the sub_level will no longer exist in the compaciton group, so it is correct to use FIRST_SUB_LEVEL_ID for initialisation.

Li0k · 2024-10-15T08:40:33Z

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

@@ -883,7 +886,7 @@ impl HummockVersion {
            )
        });

-        group_split::merge_levels(left_levels, right_levels);
+        group_split::merge_levels(left_levels, right_levels, self.max_sub_level_id);


I prefer global uniqueness, for me at the moment this sub level id has no more semantics. So globally unique is more concise.

src/storage/hummock_sdk/src/compaction_group/mod.rs

hzxa21 · 2024-10-17T09:25:09Z

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

+                .latest_version()
+                .max_sub_level_id
+                .map(|id| id + 1)
+                .unwrap_or(FIRST_SUB_LEVEL_ID);


For extra safety, I suggest we fill in max_sub_level_id using Epoch::now() if it is None on meta startup (in load_meta_store_state_impl) after all existing deltas are applied. In this case, we can make sure max_sub_level_id is larger than any previously generated sub level id.

Yes this should be the correct way.
Previously I didn't consider the delta replay for backward-compatibility. Backward compatibility shouldn't be done during pb deserialization.

Refactored:

In-mem max_sub_level_id's type is u64 instead of Option now.

backward compatibility is ensured during load_meta_store_state_impl.

@Li0k

zwang28 · 2024-10-18T02:58:07Z

I prefer #18893 to this PR.

feat(meta): commit epoch in separate group delta #18893 removes summarize_group_deltas.
The problem with summarize_group_deltas is it loses the order of events inside a delta. e.g. previously in apply_version_delta, GroupConstruct/GroupMerge is hard coded to always be applied before IntraLevel. I think such assumption of order in apply_version_delta increases the risk of bug, e.g. in case one day there is a [IntraLevel, GroupMerge, IntraLevel] delta, the hard coded order becomes incorrect.
We can always apply this PR anytime when non-decreasing order of sub_level_id is required in the future.

WDYT @hzxa21 @Li0k @wenym1

zwang28 added 3 commits October 15, 2024 12:39

Revert "fix(storage): fix sub level id for time travel (#18886)"

8d6c2af

This reverts commit 798896f.

minor refactor

9b04349

add max_sub_level_id field

0620d94

zwang28 requested review from Li0k and wenym1 October 15, 2024 06:07

zwang28 marked this pull request as ready for review October 15, 2024 06:07

github-actions bot added type/refactor ci/run-e2e-single-node-tests labels Oct 15, 2024

zwang28 added 2 commits October 15, 2024 14:14

add comment

c654974

fmt

b8bdf81

hzxa21 reviewed Oct 15, 2024

View reviewed changes

wenym1 reviewed Oct 15, 2024

View reviewed changes

bugfix

d5ecedd

Li0k approved these changes Oct 15, 2024

View reviewed changes

zwang28 added 3 commits October 15, 2024 16:53

bugfix

24e46b8

bugfix

151c84d

fix test

74d130e

zwang28 marked this pull request as draft October 15, 2024 10:02

This comment was marked as resolved.

Sign in to view

zwang28 added 2 commits October 15, 2024 18:35

refactor

d0ba96c

fix tests

d8ead5a

zwang28 marked this pull request as ready for review October 15, 2024 10:39

Li0k reviewed Oct 16, 2024

View reviewed changes

src/storage/hummock_sdk/src/compaction_group/mod.rs Outdated Show resolved Hide resolved

hzxa21 reviewed Oct 17, 2024

View reviewed changes

hzxa21 added the need-cherry-pick-release-2.1 label Oct 17, 2024

refactor

09d5b6b

zwang28 requested review from Li0k and hzxa21 October 17, 2024 12:38

zwang28 removed the need-cherry-pick-release-2.1 label Oct 18, 2024

zwang28 closed this Oct 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(storage): maintain sub_level id order #18915

refactor(storage): maintain sub_level id order #18915

zwang28 commented Oct 15, 2024

hzxa21 Oct 15, 2024

zwang28 Oct 15, 2024 •

edited

Loading

zwang28 Oct 15, 2024 •

edited

Loading

Li0k Oct 15, 2024

zwang28 Oct 15, 2024

wenym1 Oct 15, 2024

zwang28 Oct 15, 2024

wenym1 Oct 15, 2024

zwang28 Oct 15, 2024

Li0k Oct 15, 2024

Li0k left a comment

Li0k Oct 15, 2024

Li0k Oct 15, 2024

This comment was marked as resolved.

hzxa21 Oct 17, 2024

zwang28 Oct 17, 2024

zwang28 Oct 17, 2024 •

edited

Loading

zwang28 commented Oct 18, 2024 •

edited

Loading

refactor(storage): maintain sub_level id order #18915

refactor(storage): maintain sub_level id order #18915

Conversation

zwang28 commented Oct 15, 2024

What's changed and what's your intention?

Checklist

Documentation

Release note

Choose a reason for hiding this comment

zwang28 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

zwang28 Oct 15, 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

Choose a reason for hiding this comment

Li0k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwang28 Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

zwang28 commented Oct 18, 2024 • edited Loading

zwang28 Oct 15, 2024 •

edited

Loading

zwang28 Oct 15, 2024 •

edited

Loading

zwang28 Oct 17, 2024 •

edited

Loading

zwang28 commented Oct 18, 2024 •

edited

Loading