refactor(storage): use ReadOptions and WriteOptions in StateStore. #3216

zwang28 · 2022-06-14T10:14:48Z

What's changed and what's your intention?

Use ReadOptions and WriteOptions to wrap parameters for StateStore.

Store state table id in KeySpace, in order to use it in state store's read/write.
Major code changes is quite small in store.rs, but lots of call sites are affected.

Checklist

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

Refer to a related PR or issue link (optional)

#2065

codecov · 2022-06-14T10:32:40Z

Codecov Report

Merging #3216 (4281d49) into main (93e02e7) will increase coverage by 0.00%.
The diff coverage is 74.00%.

@@           Coverage Diff            @@
##             main    #3216    +/-   ##
========================================
  Coverage   73.74%   73.74%            
========================================
  Files         768      768            
  Lines      105736   106454   +718     
========================================
+ Hits        77972    78507   +535     
- Misses      27764    27947   +183

Flag	Coverage Δ
rust	`73.74% <74.00%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
src/bench/ss_bench/operations/get.rs	`0.00% <0.00%> (ø)`
...rc/bench/ss_bench/operations/prefix_scan_random.rs	`0.00% <0.00%> (ø)`
src/bench/ss_bench/operations/write_batch.rs	`0.00% <0.00%> (ø)`
src/ctl/src/cmd_impl/hummock/list_kv.rs	`0.00% <0.00%> (ø)`
src/storage/src/monitor/monitored_store.rs	`1.94% <0.00%> (-0.07%)`	⬇️
src/storage/src/panic_store.rs	`0.00% <0.00%> (ø)`
src/storage/src/store.rs	`54.54% <50.00%> (-12.13%)`	⬇️
src/storage/src/write_batch.rs	`68.62% <65.00%> (-0.27%)`	⬇️
src/storage/src/hummock/state_store_tests.rs	`76.25% <73.61%> (-3.00%)`	⬇️
src/storage/src/keyspace.rs	`82.22% <75.00%> (-3.65%)`	⬇️
... and 14 more

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

BugenZhao · 2022-06-14T10:53:01Z

Why there's a TableId inside the Keyspace interface?

zwang28 · 2022-06-14T10:55:54Z

Why there's a TableId inside the Keyspace interface?

It is cell table id and will be used to decide the compaction group during state store's read/write.

BugenZhao · 2022-06-14T10:57:15Z

Why there's a TableId inside the Keyspace interface?

It is cell table id and will be used to decide the compaction group during state store's read/write.

So is this PR a continuation of #1812? It still seems to expose too many implementation details. 😢

wenym1 · 2022-06-16T02:41:33Z

Seems that we have a lot of merge conflict right now.

hzxa21 · 2022-06-16T08:36:03Z

Why there's a TableId inside the Keyspace interface?

It is cell table id and will be used to decide the compaction group during state store's read/write.

So is this PR a continuation of #1812? It still seems to expose too many implementation details. 😢

StateTable has table_id and StateStore needs table_id for reads/writes. The issue here is how to pass StateTable's table_id to StateStore. How about putting table_id in WriteBatch instead of Keyspace?

zwang28 · 2022-06-17T06:40:47Z

How about putting table_id in WriteBatch instead of Keyspace

But state store's read interfaces still require table_id?

zwang28 · 2022-06-17T07:24:44Z

I resolved the conflict and left the test unfixed for now.
@hzxa21 WriteBatch now store table_id. But KeySpace still keep table_id to use in read path.
@BugenZhao KeySpace's interface is not modified. Would you explain which "implementation details" we don't want to expose?

hzxa21 · 2022-06-17T07:27:08Z

How about putting table_id in WriteBatch instead of Keyspace

But state store's read interfaces still require table_id?

You are right. We either pass table_id to the Keyspace read interface or store it in Keyspace just like this PR. Considering this, I think storing it in Keyspace is okay. @BugenZhao What do you think?

zwang28 · 2022-06-21T06:12:39Z

@skyzh Would you please take a look at this refactor?

Little-Wallace · 2022-06-21T06:26:31Z

src/bench/ss_bench/operations/get.rs

+                    let val_size = match store
+                        .get(
+                            &key,
+                            ReadOptions {


how about use the same ReadOptions with Hummock? (for prefetch)

You mean reusing the existed hummock ReadOptions for state store interfaces?
I think it's a concept in state store layer(although other state store impl doesn't make use of it)?

No. In the future we may need executor to tell us which it is a large scan request so that storage can decide whether we need to prefetch more data from object storage.

skyzh

Rest LGTM

skyzh · 2022-06-21T06:40:51Z

src/stream/src/executor/hash_agg.rs

        // --- Flush states to the state store ---
        // Some state will have the correct output only after their internal states have been
        // fully flushed.
        let (write_batch, dirty_cnt) = {
-            let mut write_batch = store.start_write_batch();
+            let mut write_batch = store.start_write_batch(WriteOptions { epoch, table_id });


I'm afraid this is not correct. HashAgg uses a lot of tables, we should produce multiple write batches in hash agg.

Right both hash_agg.rs and global_simple_agg.rs have been refactored after this PR. Will fix.

skyzh · 2022-06-21T06:40:58Z

src/stream/src/executor/global_simple_agg.rs

@@ -202,6 +203,7 @@ impl<S: StateStore> SimpleAggExecutor<S> {
    ) -> StreamExecutorResult<Option<StreamChunk>> {
        // The state store of each keyspace is the same so just need the first.
        let store = keyspace[0].state_store();
+        let table_id = keyspace[0].table_id();


# Conflicts: # src/ctl/src/cmd_impl/hummock/list_kv.rs # src/storage/src/hummock/state_store.rs # src/storage/src/store.rs # src/stream/src/executor/global_simple_agg.rs # src/stream/src/executor/hash_agg.rs

skyzh

LGTM. cc @BugenZhao are you working on string agg now? Seems that this PR might affect how string_agg stores and reads things. As it is not covered by our e2e test, you might want to pay attention to this PR.

# Conflicts: # src/storage/src/table/cell_based_table.rs # src/storage/src/table/test_relational_table.rs

github-actions bot added the type/refactor label Jun 14, 2022

zwang28 requested review from BugenZhao and hzxa21 June 14, 2022 10:17

zwang28 marked this pull request as ready for review June 14, 2022 10:25

BugenZhao requested review from skyzh and wenym1 June 14, 2022 10:57

WIP add ReadOptions and WriteOptions

cc40b26

zwang28 force-pushed the wangzheng/refactor_state_store branch from b7f3808 to cc40b26 Compare June 17, 2022 07:20

Little-Wallace reviewed Jun 21, 2022

View reviewed changes

skyzh reviewed Jun 21, 2022

View reviewed changes

zwang28 added 3 commits June 21, 2022 15:02

Merge branch 'main' into wangzheng/refactor_state_store

2d8c796

# Conflicts: # src/ctl/src/cmd_impl/hummock/list_kv.rs # src/storage/src/hummock/state_store.rs # src/storage/src/store.rs # src/stream/src/executor/global_simple_agg.rs # src/stream/src/executor/hash_agg.rs

fix unit tests

8a61b22

make table_id in read optional

6b8e73c

skyzh approved these changes Jun 24, 2022

View reviewed changes

zwang28 and others added 4 commits June 24, 2022 13:14

Merge branch 'main' into wangzheng/refactor_state_store

79c41e7

# Conflicts: # src/storage/src/table/cell_based_table.rs # src/storage/src/table/test_relational_table.rs

Merge branch 'main' into wangzheng/refactor_state_store

863a350

Merge branch 'main' into wangzheng/refactor_state_store

7cce88c

Merge branch 'main' into wangzheng/refactor_state_store

4281d49

zwang28 merged commit 118540a into main Jun 24, 2022

zwang28 deleted the wangzheng/refactor_state_store branch June 24, 2022 06:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(storage): use ReadOptions and WriteOptions in StateStore. #3216

refactor(storage): use ReadOptions and WriteOptions in StateStore. #3216

zwang28 commented Jun 14, 2022 •

edited

codecov bot commented Jun 14, 2022 •

edited

BugenZhao commented Jun 14, 2022

zwang28 commented Jun 14, 2022

BugenZhao commented Jun 14, 2022

wenym1 commented Jun 16, 2022

hzxa21 commented Jun 16, 2022

zwang28 commented Jun 17, 2022

zwang28 commented Jun 17, 2022 •

edited

hzxa21 commented Jun 17, 2022

zwang28 commented Jun 21, 2022

Little-Wallace Jun 21, 2022

zwang28 Jun 21, 2022 •

edited

Little-Wallace Jun 24, 2022

skyzh left a comment

skyzh Jun 21, 2022

zwang28 Jun 21, 2022

zwang28 Jun 21, 2022

skyzh Jun 21, 2022

zwang28 Jun 21, 2022

skyzh left a comment

refactor(storage): use ReadOptions and WriteOptions in StateStore. #3216

refactor(storage): use ReadOptions and WriteOptions in StateStore. #3216

Conversation

zwang28 commented Jun 14, 2022 • edited

What's changed and what's your intention?

Checklist

Refer to a related PR or issue link (optional)

codecov bot commented Jun 14, 2022 • edited

Codecov Report

BugenZhao commented Jun 14, 2022

zwang28 commented Jun 14, 2022

BugenZhao commented Jun 14, 2022

wenym1 commented Jun 16, 2022

hzxa21 commented Jun 16, 2022

zwang28 commented Jun 17, 2022

zwang28 commented Jun 17, 2022 • edited

hzxa21 commented Jun 17, 2022

zwang28 commented Jun 21, 2022

Little-Wallace Jun 21, 2022

Choose a reason for hiding this comment

zwang28 Jun 21, 2022 • edited

Choose a reason for hiding this comment

Little-Wallace Jun 24, 2022

Choose a reason for hiding this comment

skyzh left a comment

Choose a reason for hiding this comment

skyzh Jun 21, 2022

Choose a reason for hiding this comment

zwang28 Jun 21, 2022

Choose a reason for hiding this comment

zwang28 Jun 21, 2022

Choose a reason for hiding this comment

skyzh Jun 21, 2022

Choose a reason for hiding this comment

zwang28 Jun 21, 2022

Choose a reason for hiding this comment

skyzh left a comment

Choose a reason for hiding this comment

zwang28 commented Jun 14, 2022 •

edited

codecov bot commented Jun 14, 2022 •

edited

zwang28 commented Jun 17, 2022 •

edited

zwang28 Jun 21, 2022 •

edited