feat(storage): enable static compaction group #3453

zwang28 · 2022-06-24T08:06:24Z

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

Enables e2e static compaction group feature. There're 2 groups for
- materialized view and materialized source.
- internal state and source.
Assert table ids given to commit_epoch all have been registered to compaction group.

Checklist

I have written necessary docs and comments
I have added necessary unit tests and integration tests
All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

#2065

codecov · 2022-06-24T08:45:30Z

Codecov Report

Merging #3453 (bac0835) into main (86c2105) will increase coverage by 0.01%.
The diff coverage is 74.08%.

@@            Coverage Diff             @@
##             main    #3453      +/-   ##
==========================================
+ Coverage   74.29%   74.31%   +0.01%     
==========================================
  Files         773      773              
  Lines      109551   109868     +317     
==========================================
+ Hits        81391    81646     +255     
- Misses      28160    28222      +62

Flag	Coverage Δ
rust	`74.31% <74.08%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
src/bench/ss_bench/main.rs	`0.65% <0.00%> (ø)`
src/compute/src/rpc/service/stream_service.rs	`0.00% <0.00%> (ø)`
src/compute/src/server.rs	`0.00% <0.00%> (ø)`
src/ctl/src/cmd_impl/hummock/sst_dump.rs	`0.00% <0.00%> (ø)`
src/meta/src/hummock/error.rs	`2.56% <ø> (ø)`
...rc/storage/hummock_sdk/src/compaction_group/mod.rs	`100.00% <ø> (+34.61%)`	⬆️
src/storage/src/hummock/test_utils.rs	`83.91% <0.00%> (-1.80%)`	⬇️
src/storage/src/monitor/monitored_store.rs	`1.74% <0.00%> (-0.08%)`	⬇️
src/storage/src/store_impl.rs	`7.54% <0.00%> (-0.46%)`	⬇️
src/stream/src/task/stream_manager.rs	`0.00% <0.00%> (ø)`
... and 35 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

src/storage/src/hummock/state_store.rs

Little-Wallace · 2022-06-25T06:13:26Z

src/storage/src/hummock/compaction_group_client.rs

    /// Tries to get from local cache
-    pub async fn try_get_compaction_group_id(&self, prefix: Prefix) -> Option<CompactionGroupId> {
+    async fn try_get_compaction_group_id(&self, prefix: Prefix) -> Option<CompactionGroupId> {


Do not async RwLock because any RPC request will block other read request.

suggest parking_lot::RwLock to reduce lock wait.

let mut guard = self.inner.read(); if let Some(compaction_group_id) = guard.get(&prefix) { return Ok(Some(compaction_group_id)); } drop(guard); let compaction_groups = self .hummock_meta_client .get_compaction_groups() .await .map_err(HummockError::meta_error)?; let mut guard = self.inner.write(); guard.set_index(compaction_groups); Ok(guard.get(&prefix))

Then we also need to avoid duplicate RPCs. It's doable.

I used the current approach because I think cache update is rare, which is only required when new table is created.

need to avoid duplicate RPCs

in this case get_compaction_groups is a read operation , why we need to avoid duplicate RPCs (to reduce read rpc ?)

No. This RPC is light and cost little CPU of meta-service. So we do not need to avoid duplicate RPCs

# Conflicts: # src/meta/src/hummock/compaction_group/manager.rs

…uld not be unregistered from compaction group.

# Conflicts: # src/meta/src/stream/source_manager.rs # src/storage/src/hummock/compactor_tests.rs

# Conflicts: # src/storage/src/hummock/state_store.rs

hzxa21 · 2022-06-27T12:31:06Z

src/storage/src/hummock/compaction_group_client.rs

+#[async_trait::async_trait]
+impl CompactionGroupClient for CompactionGroupClientImpl {
+    /// Tries to get from local cache first,then from meta service
+    async fn get_compaction_group_id(


If shared buffer is already split by compaction group, I have a feeling that we will need to know more than just a compaction group id, for example, the compaction group configuration.

i think so, hope that CompactionGroupClient can fetch more info when rpc, (link configuration) , in order to support ttl and so on

The complete compaction group info is already given to CompactionGroupClient, but CompactionGroupClient only store part of it for now.
I think you can modify it as need later @Li0k .

src/storage/src/hummock/state_store.rs

hzxa21

All my comments are concerns on future PRs and not related to "static" compaction group. You can merge this PR first if you want.

# Conflicts: # src/storage/src/hummock/local_version_manager.rs

zwang28 · 2022-06-28T03:20:01Z

@Little-Wallace suggests to update CN's table->group mapping in a more timely manner, rather than current lazy manner. It makes sense to me. For example, we can update this mapping in CN when CN is process conf change barrier.

Also there is an unstable failure parallel e2e test in main. Would only merge this PR after it has been fixed.

zwang28 · 2022-06-28T06:19:56Z

For example, we can update this mapping in CN when CN is process conf change barrier.

~~After some attempts, I now suggest to keep the lazy update approach for now. Because to implement aforementioned more timely approach:~~
~~- We need update mapping in LocalStreamManager::send_barrier. It is not async, while update RPC requires async.~~
~~- Alternatively it's possible to piggyback the mapping in barrier payload, thus async update RPC is avoided. But I prefer not to complicate barrier.~~

zwang28 · 2022-06-28T21:26:37Z

~~The rpc deduplication implementation is flawed. Will fix before merge.~~
We update cache in the beginning of build_actors. Because it can avoid RPC duplication better.

# Conflicts: # src/meta/src/hummock/hummock_manager.rs # src/storage/hummock_sdk/src/compaction_group/hummock_version_ext.rs

zwang28 · 2022-06-30T09:18:40Z

A summary of changes since last review @Li0k @Little-Wallace @hzxa21 :

CN's compaction group cache is updated in the beginning of build_actors each time, rather than lazily updated when queried.
Remove prefix concept in compaction group. Use table id directly.
CompactionGroupClient is quite simple now.

Li0k

Thanks for the PR. LGTM!

# Conflicts: # src/ctl/src/cmd_impl/hummock/sst_dump.rs # src/meta/src/hummock/compaction_group/manager.rs # src/meta/src/hummock/compaction_group/mod.rs # src/storage/src/hummock/compactor_tests.rs # src/storage/src/hummock/state_store.rs

hzxa21 · 2022-07-01T08:01:30Z

src/meta/src/stream/stream_manager.rs

+        self.compaction_group_manager
+            .register_table_fragments(&table_fragments, &table_properties)
+            .await?;
+
        // In the second stage, each [`WorkerNode`] builds local actors and connect them with
        // channels.
        for (node_id, actors) in node_actors {


Do we need to unregister_table_fragments if BuildActorsRequest fails?

purge_stale_members ensures stale data is eventually cleaned up on each restart.
We can unregister_table_fragments/unregister_source explicitly to clean up more timely. I didn't have it here, in order to avoid introducing more error handling code here.
Let me add it.

# Conflicts: # src/workspace-hack/Cargo.toml

zwang28 force-pushed the wangzheng/enable_compaction_group branch from 1176775 to 93a7d7b Compare June 24, 2022 08:18

feat(storage): enable static compaction group

90d191c

zwang28 force-pushed the wangzheng/enable_compaction_group branch from 93a7d7b to 90d191c Compare June 24, 2022 08:33

Little-Wallace reviewed Jun 24, 2022

View reviewed changes

src/storage/src/hummock/state_store.rs Outdated Show resolved Hide resolved

Little-Wallace reviewed Jun 25, 2022

View reviewed changes

zwang28 added 2 commits June 26, 2022 16:10

assert table_ids are already registered in commit_epoch

8ca70f2

Merge branch 'main' into wangzheng/enable_compaction_group

293e533

# Conflicts: # src/meta/src/hummock/compaction_group/manager.rs

github-actions bot added the type/feature label Jun 26, 2022

fix issue: when source is dropped but still used in fragments, it sho…

a1bca80

…uld not be unregistered from compaction group.

zwang28 marked this pull request as ready for review June 26, 2022 09:26

zwang28 marked this pull request as draft June 27, 2022 04:53

zwang28 added 2 commits June 27, 2022 14:29

refactor CompactionGroupClient

a795031

Merge branch 'main' into wangzheng/enable_compaction_group

c4a81cd

# Conflicts: # src/meta/src/stream/source_manager.rs # src/storage/src/hummock/compactor_tests.rs

zwang28 marked this pull request as ready for review June 27, 2022 08:52

zwang28 requested a review from hzxa21 June 27, 2022 08:52

zwang28 added 2 commits June 27, 2022 17:00

add comments

011e15e

Merge branch 'main' into wangzheng/enable_compaction_group

8053f4d

# Conflicts: # src/storage/src/hummock/state_store.rs

hzxa21 reviewed Jun 27, 2022

View reviewed changes

src/storage/src/hummock/state_store.rs Show resolved Hide resolved

hzxa21 approved these changes Jun 27, 2022

View reviewed changes

zwang28 added 2 commits June 28, 2022 10:33

Merge branch 'main' into wangzheng/enable_compaction_group

3a3fe40

# Conflicts: # src/storage/src/hummock/local_version_manager.rs

add comments for static compaction group

dab54d8

zwang28 added 2 commits June 30, 2022 11:10

update compaction group cache on barrier

6b38edf

use table id directly, instead of additional prefix.

cab82cb

zwang28 force-pushed the wangzheng/enable_compaction_group branch from 040f551 to cab82cb Compare June 30, 2022 03:46

update compaction group cache before build actors

fb22002

zwang28 added 2 commits June 30, 2022 16:22

Merge branch 'main' into wangzheng/enable_compaction_group

0bb4bf5

# Conflicts: # src/meta/src/hummock/hummock_manager.rs # src/storage/hummock_sdk/src/compaction_group/hummock_version_ext.rs

register compaction group for MV before build_actors

1859a3d

zwang28 force-pushed the wangzheng/enable_compaction_group branch from c0d5fbd to 1859a3d Compare June 30, 2022 08:58

zwang28 requested review from Li0k, hzxa21 and Little-Wallace June 30, 2022 09:18

fix comment

607a12f

Li0k approved these changes Jul 1, 2022

View reviewed changes

hzxa21 reviewed Jul 1, 2022

View reviewed changes

zwang28 added 4 commits July 2, 2022 16:35

revert compaction group on error.

7f19bf4

fix check

4444ba0

fix comment

4d8fee1

Merge branch 'main' into wangzheng/enable_compaction_group

bac0835

# Conflicts: # src/workspace-hack/Cargo.toml

hzxa21 approved these changes Jul 4, 2022

View reviewed changes

zwang28 merged commit dc165bb into main Jul 4, 2022

zwang28 deleted the wangzheng/enable_compaction_group branch July 4, 2022 04:24

huangjw806 pushed a commit that referenced this pull request Jul 5, 2022

feat(storage): enable static compaction group (#3453)

081f46a

nasnoisaac pushed a commit to nasnoisaac/risingwave that referenced this pull request Aug 9, 2022

feat(storage): enable static compaction group (risingwavelabs#3453)

716a9cf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage): enable static compaction group #3453

feat(storage): enable static compaction group #3453

zwang28 commented Jun 24, 2022 •

edited

codecov bot commented Jun 24, 2022 •

edited

Little-Wallace Jun 25, 2022

Little-Wallace Jun 25, 2022

Little-Wallace Jun 25, 2022

zwang28 Jun 25, 2022 •

edited

Li0k Jun 25, 2022

Little-Wallace Jun 27, 2022

zwang28 Jun 27, 2022

hzxa21 Jun 27, 2022

Li0k Jun 27, 2022

zwang28 Jun 28, 2022

hzxa21 left a comment

zwang28 commented Jun 28, 2022 •

edited

zwang28 commented Jun 28, 2022 •

edited

zwang28 commented Jun 28, 2022 •

edited

zwang28 commented Jun 30, 2022 •

edited

Li0k left a comment

hzxa21 Jul 1, 2022

zwang28 Jul 1, 2022 •

edited

zwang28 Jul 2, 2022

feat(storage): enable static compaction group #3453

feat(storage): enable static compaction group #3453

Conversation

zwang28 commented Jun 24, 2022 • edited

What's changed and what's your intention?

Checklist

Refer to a related PR or issue link (optional)

codecov bot commented Jun 24, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwang28 Jun 25, 2022 • edited

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 left a comment

Choose a reason for hiding this comment

zwang28 commented Jun 28, 2022 • edited

zwang28 commented Jun 28, 2022 • edited

zwang28 commented Jun 28, 2022 • edited

zwang28 commented Jun 30, 2022 • edited

Li0k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwang28 Jul 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwang28 commented Jun 24, 2022 •

edited

codecov bot commented Jun 24, 2022 •

edited

zwang28 Jun 25, 2022 •

edited

zwang28 commented Jun 28, 2022 •

edited

zwang28 commented Jun 28, 2022 •

edited

zwang28 commented Jun 28, 2022 •

edited

zwang28 commented Jun 30, 2022 •

edited

zwang28 Jul 1, 2022 •

edited