fix(meta): do not split by vnode for low write throughput #12534

Little-Wallace · 2023-09-26T08:04:10Z

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

What's changed and what's your intention?

If a group does not write much data after split, we may generate too many small files in level-0. This PR will fix this case by control whether to cut sstable by vnode partition

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
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.

Signed-off-by: Little-Wallace <[email protected]>

codecov · 2023-09-26T08:54:08Z

Codecov Report

Attention: 91 lines in your changes are missing coverage. Please review.

Comparison is base (5c588ef) 67.99% compared to head (b3433d5) 67.95%.
Report is 33 commits behind head on main.

Files	Patch %	Lines
...mmock/compaction/picker/intra_compaction_picker.rs	33.66%	67 Missing ⚠️
src/meta/src/hummock/manager/mod.rs	82.75%	10 Missing ⚠️
.../compaction/picker/base_level_compaction_picker.rs	69.23%	8 Missing ⚠️
...ck_sdk/src/compaction_group/hummock_version_ext.rs	86.20%	4 Missing ⚠️
src/meta/src/hummock/compaction/picker/mod.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12534      +/-   ##
==========================================
- Coverage   67.99%   67.95%   -0.05%     
==========================================
  Files        1535     1535              
  Lines      264830   264992     +162     
==========================================
  Hits       180080   180080              
- Misses      84750    84912     +162

Flag	Coverage Δ
rust	`67.95% <72.67%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Little-Wallace <[email protected]>

Little-Wallace · 2023-10-20T03:01:47Z

Little-Wallace · 2023-10-20T03:27:35Z

Although this PR reduce many small file but I find that it may cause more large task and the write-amplification also increase a little.

Signed-off-by: Little-Wallace <[email protected]>

Little-Wallace · 2023-10-30T03:38:51Z

The problem which choose too many files in compact task 0-->base has been fixed.

To solve this problem, I refactor the score calculate to prevent no split level compact to base level.

src/meta/src/hummock/compaction/picker/compaction_task_validator.rs

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

Li0k · 2023-10-30T10:17:43Z

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

@@ -747,6 +773,10 @@ impl HummockLevelsExt for Levels {
        }
        delete_sst_ids_set.is_empty()
    }
+
+    fn can_partition_by_vnode(&self) -> bool {


Nothing to do with the current PR, in another PR I'm going to turn on partitioning in the default cg and the self.member_table_ids.len() == 1 will fail, any suggestions?

Li0k · 2023-10-30T10:18:46Z

@zwang28 @hzxa21 PTAL

Signed-off-by: Little-Wallace <[email protected]>

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

zwang28

I haven't finished the review yet.

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

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

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

zwang28 · 2023-11-01T06:13:50Z

proto/hummock.proto

@@ -116,6 +118,7 @@ message HummockVersion {
    uint64 group_id = 3;
    uint64 parent_group_id = 4;
    repeated uint32 member_table_ids = 5;
+    uint32 vnode_partition_count = 6;


Would you explain when this field is initialized / set?

set in function build_initial_compaction_group_levels.

So for existing compaction groups, vnode_partition_count is 0 and cannot be changed.

🤔 Good catch.
I will refactor this code to avoid compatibility issues

zwang28

Rest LGTM.
Will partition_vnode_count be mutable someday?

zwang28 · 2023-11-09T14:24:25Z

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


            // Reduce the level num of l0 non-overlapping sub_level
-            ctx.score_levels.push({
-                PickerInfo {
+            if non_overlapping_size_score > SCORE_BASE {


Could you explain the idea behind non_overlapping_size_score > SCORE_BASE and non_overlapping_level_score > SCORE_BASE? Why ToBase is stricter about size_score, while Intra is stricter about level_score?

because if there are not enough split-vnode file in level0, the selector will pick a large task to base level and it will case more write-amplification and also slowdown compaction

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

Li0k · 2023-12-12T04:50:39Z

PTAL @hzxa21

Li0k · 2023-12-12T08:37:19Z

Investigate and avoid small SSTs in bottom level #13876

hzxa21 · 2023-12-12T10:00:40Z

There are several changes in main recently. I think we can rebase main and re-run the experiments to verify the results of this PR.

Signed-off-by: Little-Wallace <[email protected]>

Little-Wallace · 2023-12-18T06:47:25Z

There are several changes in main recently. I think we can rebase main and re-run the experiments to verify the results of this PR.

I run a benchmark test for nexmark q5,q7,q9,q10,q15,q16,q19,q20. It does not matter for performance

…2534)" This reverts commit 6c5b56d.

Signed-off-by: Little-Wallace <[email protected]>

do not split too many files

3dcff6b

Signed-off-by: Little-Wallace <[email protected]>

github-actions bot added the type/fix Bug fix label Sep 26, 2023

fix config

35d542f

Signed-off-by: Little-Wallace <[email protected]>

Little-Wallace mentioned this pull request Sep 26, 2023

feat(compaction): partition all levels by vnode #11903

Closed

8 tasks

fix test

af781bc

Signed-off-by: Little-Wallace <[email protected]>

Little-Wallace added 5 commits September 26, 2023 22:58

Merge branch 'main' into wallace/partition-vnode

8f440fe

fix conflict

a6a5f28

Signed-off-by: Little-Wallace <[email protected]>

Merge branch 'main' into wallace/partition-vnode

2a0d45e

Merge branch 'main' into wallace/partition-vnode

4be7e79

fix metrics

950ed96

Signed-off-by: Little-Wallace <[email protected]>

Little-Wallace added 10 commits October 20, 2023 12:42

fix score

cca903b

Signed-off-by: Little-Wallace <[email protected]>

fix base level compact

dc4445f

Signed-off-by: Little-Wallace <[email protected]>

fix score

199038a

Signed-off-by: Little-Wallace <[email protected]>

do not pick no partition level

4c144ea

Signed-off-by: Little-Wallace <[email protected]>

add warn log

b8b7761

Signed-off-by: Little-Wallace <[email protected]>

fix trivial move

fc2661d

Signed-off-by: Little-Wallace <[email protected]>

remove log

c3db7bc

Signed-off-by: Little-Wallace <[email protected]>

remove warn

ee22ef5

Signed-off-by: Little-Wallace <[email protected]>

fix check

5f493ba

Signed-off-by: Little-Wallace <[email protected]>

Merge branch 'main' into wallace/partition-vnode

50e9572

Li0k self-requested a review October 30, 2023 08:17

Li0k reviewed Oct 30, 2023

View reviewed changes

address comment

f1e6aaf

Signed-off-by: Little-Wallace <[email protected]>

Li0k reviewed Oct 31, 2023

View reviewed changes

src/meta/src/hummock/compaction/selector/level_selector.rs Show resolved Hide resolved

zwang28 reviewed Nov 1, 2023

View reviewed changes

zwang28 approved these changes Nov 9, 2023

View reviewed changes

Merge branch 'main' into wallace/partition-vnode

cc4289a

Li0k approved these changes Nov 15, 2023

View reviewed changes

Little-Wallace added 6 commits December 13, 2023 13:50

Merge branch 'main' into wallace/partition-vnode

e466a1d

fix conflict

856bf4d

Signed-off-by: Little-Wallace <[email protected]>

fix test

d0eee2d

Signed-off-by: Little-Wallace <[email protected]>

add log for skip by write amp

6cf7c83

Signed-off-by: Little-Wallace <[email protected]>

do not wait too much files

4805b7f

Signed-off-by: Little-Wallace <[email protected]>

fix ut

b3433d5

Signed-off-by: Little-Wallace <[email protected]>

Merge branch 'main' into wallace/partition-vnode

a28859c

Little-Wallace added this pull request to the merge queue Jan 10, 2024

Merged via the queue into main with commit 6c5b56d Jan 10, 2024
29 of 30 checks passed

Little-Wallace deleted the wallace/partition-vnode branch January 10, 2024 09:05

Little-Wallace mentioned this pull request Jan 10, 2024

fix(compaction): fix compatible for vnode partition #14489

Merged

9 tasks

cyliu0 mentioned this pull request Jan 16, 2024

ch-benchmark modified q2 result different to upstream PG #14592

Closed

Little-Wallace added a commit that referenced this pull request Jan 22, 2024

Revert "fix(meta): do not split by vnode for low write throughput (#1…

a39a83b

…2534)" This reverts commit 6c5b56d.

Little-Wallace mentioned this pull request Jan 23, 2024

fix(hummock): fix gc delete keys incorrectly #14751

Merged

9 tasks

Little-Wallace mentioned this pull request Jan 30, 2024

Investigate and avoid small SSTs in bottom level #13876

Open

Li0k pushed a commit that referenced this pull request Mar 8, 2024

fix(meta): do not split by vnode for low write throughput (#12534)

d4bf515

Signed-off-by: Little-Wallace <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(meta): do not split by vnode for low write throughput #12534

fix(meta): do not split by vnode for low write throughput #12534

Little-Wallace commented Sep 26, 2023 •

edited

Loading

codecov bot commented Sep 26, 2023 •

edited

Loading

Little-Wallace commented Oct 20, 2023

Little-Wallace commented Oct 20, 2023

Little-Wallace commented Oct 30, 2023

Li0k Oct 30, 2023

Little-Wallace Oct 31, 2023

Li0k commented Oct 30, 2023

zwang28 left a comment

zwang28 Nov 1, 2023

Little-Wallace Nov 7, 2023

zwang28 Nov 9, 2023

Little-Wallace Dec 22, 2023

zwang28 left a comment

zwang28 Nov 9, 2023

Little-Wallace Nov 13, 2023

Li0k commented Dec 12, 2023

Li0k commented Dec 12, 2023

hzxa21 commented Dec 12, 2023

Little-Wallace commented Dec 18, 2023

fix(meta): do not split by vnode for low write throughput #12534

fix(meta): do not split by vnode for low write throughput #12534

Conversation

Little-Wallace commented Sep 26, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

codecov bot commented Sep 26, 2023 • edited Loading

Codecov Report

Little-Wallace commented Oct 20, 2023

Little-Wallace commented Oct 20, 2023

Little-Wallace commented Oct 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Li0k commented Oct 30, 2023

zwang28 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

zwang28 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Li0k commented Dec 12, 2023

Li0k commented Dec 12, 2023

hzxa21 commented Dec 12, 2023

Little-Wallace commented Dec 18, 2023

Little-Wallace commented Sep 26, 2023 •

edited

Loading

codecov bot commented Sep 26, 2023 •

edited

Loading