fix(storage): fix flush small files when the capacity of shared-buffer is full #15832

Little-Wallace · 2024-03-21T06:15:02Z

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

What's changed and what's your intention?

When there are several barriers try to write data to hummock, hummock may flush a lot of small files because we flush data in barrier order instead of size order.
I show the wrong case in unit-test, and it fails in main branch.

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.

Little-Wallace · 2024-03-21T07:14:03Z

Another problem is that if there are several small upload task with epoch-1 keep running, EventHandler can not poll the tasks with epoch-2 or larger.
It means that even if a large task with epoch-2 has been finished, EventHandler must wait the small tasks finished before and then it could recycle the memory quota of this large task. And because of this problem, the metrics would shows that the usage of shared-buffer full for a long time.
cc @wenym1

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

hzxa21 · 2024-03-28T02:42:43Z

src/storage/src/hummock/event_handler/uploader.rs

            .iter()
            .flat_map(|imm| imm.epochs().clone())
            .sorted()
            .dedup()
            .collect_vec();

-        // reverse to make newer epochs comes first
-        epochs.reverse();


Will there be any side-effect for this change? cc @wenym1

src/storage/src/hummock/event_handler/uploader.rs

src/storage/src/hummock/store/local_hummock_storage.rs

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

src/storage/src/hummock/store/version.rs

hzxa21 · 2024-04-17T04:13:48Z

src/storage/src/hummock/store/version.rs

                        .iter()
-                        .rev()
-                        .is_sorted_by_key(|imm| imm.batch_id()));


I think the imm_ids in self.staging.data are still sorted. Why do we remove this assertion?

Because some of them may be sstable file, and sstable file does not have a batch id.

src/storage/src/hummock/store/version.rs

hzxa21 · 2024-04-17T04:29:28Z

src/storage/src/hummock/store/version.rs

+                        )
+                }
+                StagingData::Sst(staging_sst) => {
+                    let sst_min_epoch = *staging_sst.epochs.first().expect("epochs not empty");


Why do we use the max_epoch in staging sst previously? Sounds like a bug. cc @Li0k

No. If the min epoch is smaller than max_epoch_inclusive, we think the data range may be overlap with query range

Sorry for the confusion. I mean using the max_epoch previously is a bug. Using min_epoch is the correct way.

Good Catch, use min_epoch to filter is a better way, even if we don’t have such a usage scenario now

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

hzxa21 · 2024-04-18T06:06:57Z

src/storage/src/hummock/store/version.rs

+                            StagingData::Sst(_) => true,
+                        });
+                        self.staging.data.push(StagingData::Sst(staging_sst_ref));
+                        self.staging.data.sort_by_key(|data| data.min_epoch());


If we have more than one imm/sst belong to the same epoch, will sorting by epoch breaks the correct ordering? I still think sorting by epoch is a bit risky. Given that we have found the consecutive IMMs to be replaced, isn't it more straight-forward to replace these IMMs with the staging SST instead of pushing staging sst to the end and sorting by min epoch?

@wenym1 Since that we add offset to tail of epoch, could we assume that every memtable own different epoch?

Yes, every memtable of a single local state store instance owns different epochs. But I'm afraid that we cannot use the min_epoch of sst here.

Let's use epoch<a>-<b> to represent an EpochWithGap with real epoch to be a and inner offset as b, and we have two instances (A and B) of local state store.

In epoch1, A writes an imm with epoch1-1, and then a spill happened and generated a SST with min epoch as epoch1-1. And then B writes an imm with epoch1-1, and then A writes an imm with epoch1-2, and then a spill happened and generated a SST with min epoch also as epoch1-1. Both ssts contains the data of A, so they will both appear in A, but they have the same min epoch as epoch1-1, and this will cause some order problem if we simply sort by min epoch.

I think #15832 (comment) can solve the problem in a more elegant way, and we can avoid the sorting to reduce the time.

Anyway, I refactor this code and replace the memtable with staing-data rather than sort all array.

I think refactor it with BTreeMap can left in next PR

src/common/src/config.rs

src/storage/src/hummock/event_handler/uploader.rs

src/storage/src/hummock/store/version.rs

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

src/storage/src/hummock/store/version.rs

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

hzxa21 · 2024-04-25T12:18:53Z

Discussed offline: Given that 1.9 will re-enabled fast compaction via #15286. Let's release this improvement PR in 1.10 instead of 1.9. We can hold the PR merge until 1.9 is released.

wenym1 · 2024-05-17T05:16:26Z

src/storage/src/hummock/store/version.rs

+                        self.staging
+                            .data
+                            .insert(first_pos, StagingData::Sst(staging_sst_ref));
+                        self.staging.data.retain(|data| match data {


retain will iterate over all imms and ssts. It has been observed that when there are many actors, this cost will be amplified greatly, and cause the event handler stuck. With #16725 merged, in staging_sst_ref, we can now know what imm id should be replaced. We shall change to maintain a btreemap on the staging data as suggested in the previous comment.

Good catch. I will refactor it later.

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

hzxa21 · 2024-05-27T05:46:42Z

src/storage/src/hummock/event_handler/uploader.rs

        }
+        let mut flush_unseal = self.sealed_data.flush(&self.context, true);


nits: this triggers flush for sealed_data so it is better call it flushed instead of flush_unseal

hzxa21 · 2024-05-27T05:56:41Z

src/storage/src/hummock/event_handler/uploader.rs

@@ -1433,8 +1457,14 @@ mod tests {
    }

    #[tokio::test]
-    async fn test_uploader_finish_in_order() {
-        let (buffer_tracker, mut uploader, new_task_notifier) = prepare_uploader_order_test();
+    async fn test_uploader_finish_not_in_order() {


The implementation of test_uploader_finish_in_order is unchanged, meaning that it is still testing "finish_in_order". Can we preserve the previous name?

hzxa21 · 2024-05-27T06:02:50Z

src/storage/src/hummock/event_handler/uploader.rs

+        }
+        let imm = gen_imm_with_limiter(epoch1, Some(memory_limiter.as_ref())).await;
+        uploader.add_imm(imm);
+        assert!(uploader.may_flush());


In addition to checking whether may_flush is triggered, we should assert that epoch2's data is flushed instead of epoch1. IIUC, may_flush will triggered with and without this PR. The real behavior change is epoch2 will be flushed first before epoch1.

hzxa21 · 2024-05-27T08:56:03Z

After discussing with @wenym1 offline, we found that there is a critical pitfall of the implementation of this PR, which can affect the correctness. Let's use imm(<table_id>, <epoch>) to denote a IMM. Consider the following events in sequence:

state table t1 adds a small imm(t1, epoch1) to uploader.
state table t2 adds a small imm(t2, epoch1) to uploader.
state table t1 adds a large imm(t1, epoch2) to uploader.
state table t2 adds a small imm(t2, epoch2) to uploader.
state table t1 adds a small imm(t1, epoch3) to uploader.
state table t2 adds a small imm(t2, epoch3) to uploader.
shared buffer flush threshold is reached and epoch2's imm(t1, epoch2) and imm(t2, epoch2) are spilled to SST1
assuming epoch1 and epoch2 are non-checkpoint epoch and sync on epoch3 happens, the remaining epoch1 and epoch3's imm imm(t1, epoch1), imm(t1, epoch3), imm(t2, epoch1), imm(t2, epoch3) are spilled to SST2
SST2 is considered fresher but SST1 may contain fresher data for keys written in epoch1 and epoch2 but not in epoch3.

github-actions · 2024-08-01T02:08:13Z

This PR has been open for 60 days with no activity.

If it's blocked by code review, feel free to ping a reviewer or ask someone else to review it.

If you think it is still relevant today, and have time to work on it in the near future, you can comment to update the status, or just manually remove the no-pr-activity label.

You can also confidently close this PR to keep our backlog clean. (If no further action taken, the PR will be automatically closed after 7 days. Sorry! 🙏)
Don't worry if you think the PR is still valuable to continue in the future.
It's searchable and can be reopened when it's time. 😄

Little-Wallace requested a review from wcy-fdu March 21, 2024 06:15

Little-Wallace changed the title ~~fix(storage): flush small files when the capacity of shared-buffer is full~~ fix(storage): fix flush small files when the capacity of shared-buffer is full Mar 21, 2024

wenym1 self-requested a review March 21, 2024 07:20

fix flush small files

a144d7c

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

Little-Wallace force-pushed the wallace/reduce-flush-size branch from 0826e05 to a144d7c Compare March 25, 2024 11:37

wcy-fdu requested a review from st1page March 26, 2024 06:57

fuyufjh requested a review from hzxa21 March 26, 2024 06:59

hzxa21 reviewed Apr 8, 2024

View reviewed changes

Little-Wallace added 2 commits April 13, 2024 22:37

address comment

3d2035e

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

fix format

2a81ff9

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

hzxa21 reviewed Apr 17, 2024

View reviewed changes

Little-Wallace added 2 commits April 17, 2024 15:09

address comment

7331020

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

limit upload task count

1f0fd4c

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

hzxa21 reviewed Apr 18, 2024

View reviewed changes

wenym1 reviewed Apr 18, 2024

View reviewed changes

src/storage/src/hummock/store/version.rs Show resolved Hide resolved

Little-Wallace added 3 commits April 22, 2024 15:08

Merge branch 'main' into wallace/reduce-flush-size

c5b74d9

address comment

fac78b6

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

fix format

66b149c

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

hzxa21 reviewed Apr 22, 2024

View reviewed changes

src/storage/src/hummock/store/version.rs Show resolved Hide resolved

Little-Wallace added 4 commits April 23, 2024 13:00

revert flush limit

4e3a4a9

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

Merge branch 'main' into wallace/reduce-flush-size

370d244

Merge branch 'main' into wallace/reduce-flush-size

6c22678

fix ut

6b3486d

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

Merge branch 'main' into wallace/reduce-flush-size

301a7d3

wenym1 reviewed May 17, 2024

View reviewed changes

Little-Wallace added 3 commits May 23, 2024 12:06

Merge branch 'main' into wallace/reduce-flush-size

aad206e

fix conflict

aa8205b

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

fix tests

df7b13a

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

Little-Wallace force-pushed the wallace/reduce-flush-size branch from 0c25eb2 to df7b13a Compare May 24, 2024 03:32

hzxa21 reviewed May 27, 2024

View reviewed changes

wenym1 mentioned this pull request May 28, 2024

feat(storage): support setting minimum spill batch flush size #16962

Merged

9 tasks

github-actions bot added the no-pr-activity label Aug 1, 2024

wenym1 closed this Sep 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(storage): fix flush small files when the capacity of shared-buffer is full #15832

fix(storage): fix flush small files when the capacity of shared-buffer is full #15832

Little-Wallace commented Mar 21, 2024 •

edited

Loading

Little-Wallace commented Mar 21, 2024

hzxa21 Mar 28, 2024

hzxa21 Apr 17, 2024

Little-Wallace Apr 17, 2024

hzxa21 Apr 17, 2024

Little-Wallace Apr 17, 2024

hzxa21 Apr 18, 2024

Li0k May 17, 2024

hzxa21 Apr 18, 2024

Little-Wallace Apr 22, 2024

wenym1 Apr 22, 2024

Little-Wallace Apr 22, 2024

hzxa21 commented Apr 25, 2024

wenym1 May 17, 2024

Little-Wallace May 23, 2024

hzxa21 May 27, 2024

hzxa21 May 27, 2024

hzxa21 May 27, 2024

hzxa21 commented May 27, 2024

github-actions bot commented Aug 1, 2024

		}
		let mut flush_unseal = self.sealed_data.flush(&self.context, true);

fix(storage): fix flush small files when the capacity of shared-buffer is full #15832

fix(storage): fix flush small files when the capacity of shared-buffer is full #15832

Conversation

Little-Wallace commented Mar 21, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

Little-Wallace commented Mar 21, 2024

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

hzxa21 commented Apr 25, 2024

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 commented May 27, 2024

github-actions bot commented Aug 1, 2024

Little-Wallace commented Mar 21, 2024 •

edited

Loading