test: support `background_ddl_rate` in recovery test #13552

kwannoel · 2023-11-21T07:53:51Z

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

What's changed and what's your intention?

Materialized views without backfill leaf nodes will not run in the background after this PR. e.g. StreamValues, DeltaJoin.
This is because backfill facilitates the recovery process of mviews.
This PR also supports background mviews in deterministic test framework. It will wait for the background ddl to complete by inserting a WAIT, and making sure it is created by querying the mview catalog.
This PR also supports randomly running mviews in background for recovery test by setting BACKGROUND_DDL variable.
This PR also handles some fixes where it will handle MVs which cannot run in background_ddl mode, and properly tracks backfill actors.

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.

kwannoel · 2023-11-22T07:26:23Z

src/tests/simulation/src/slt.rs

+        bail!("failed to query pg_matviews for {mview_name}");
+    };
+
+    match result[0].try_get::<_, i64>(0) {


#13539 (comment)

Should we test against the count?

Tested here.

kwannoel · 2023-11-22T07:28:37Z

src/tests/simulation/src/slt.rs

+    /// Set background ddl
+    SetBackgroundDdl {
+        enable: bool,
+    },


#13539 (comment)

As SET will be replayed after recovery, is it okay to directly fetch the value of background_ddl with SQL?

If we have Cmd::SetBackgroundDdl, we can use it to check if background_ddl_rate is set along with manually setting SET BACKGROUND_DDL.

Both of these are incompatible, since background_ddl_rate can randomly SET BACKGROUND_DDL, and make the result of the test with SET BACKGROUND_DDL unexpected.

IMO it's not much effort to parse it.

kwannoel · 2023-11-22T07:29:45Z

src/tests/simulation/src/slt.rs

+    let sql = sql.to_lowercase();
+    let tokens = sql.split_whitespace();
+    let mut tokens = tokens.multipeek();
+    let first_token = tokens.next().unwrap_or("");


#13539 (comment)

If we have some tests intended to fail with parse error, will this lead to panic of the test driver?

Removed the unwraps.

codecov · 2023-11-22T07:45:06Z

Codecov Report

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

Comparison is base (f92b501) 68.22% compared to head (f739575) 68.24%.
Report is 16 commits behind head on main.

Files	Patch %	Lines
src/meta/src/manager/catalog/fragment.rs	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13552      +/-   ##
==========================================
+ Coverage   68.22%   68.24%   +0.02%     
==========================================
  Files        1521     1521              
  Lines      261709   261719      +10     
==========================================
+ Hits       178557   178617      +60     
+ Misses      83152    83102      -50

Flag	Coverage Δ
rust	`68.24% <66.66%> (+0.02%)`	⬆️

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.

e2e_test/background_ddl/sim/basic.slt

…y-ddl-2

kwannoel · 2023-11-27T05:26:02Z

Hi, PTAL @BugenZhao @yezizp2012

stdrc

Rest LGTM

stdrc · 2023-11-27T05:17:22Z

src/tests/simulation/src/slt.rs

+        "set" => {
+            if sql.contains("background_ddl") {
+                let enable = sql.contains("true");
+                SqlCmd::SetBackgroundDdl { enable }
+            } else {
+                SqlCmd::Others
+            }
+        }


Will this be too hard-coded?

Alternative is to use frontend to do parsing for each statement. But I choose not to do it because it may cause regression in our test runtime. So I think this is good enough for testing purpose.

yezizp2012

The functionality LGTM, but I'm not sure if it's a good practice to change the create way of every mview in each slt by chance. Previously adding kill-rate in recovery test was to reduce the total test time in CI test, it's more like a operation action for recovery test. The background_ddl_rate somehow changes the test cases I think, what if user want to run some tests in foreground on purpose?
I think maybe background_ddl tests is enough and already involved in recovery test to see if it's recoverable. And we can add some more tests under background_ddl and integration tests. 🤔

kwannoel · 2023-11-27T07:04:40Z

The functionality LGTM, but I'm not sure if it's a good practice to change the create way of every mview in each slt by chance. Previously adding kill-rate in recovery test was to reduce the total test time in CI test, it's more like a operation action for recovery test. The background_ddl_rate somehow changes the test cases I think, what if user want to run some tests in foreground on purpose? I think maybe background_ddl tests is enough and already involved in recovery test to see if it's recoverable. And we can add some more tests under background_ddl and integration tests. 🤔

I agree that user may want to run some tests in foreground on purpose, and test recovery for it.
What about if I disable it by default for the original recovery test? And rerun these tests in a separate workflow (background recovery tests) in main-cron?

This test managed to catch several bugs and I would prefer to keep it since it includes more coverage, that would take a long time to add manually.

BugenZhao · 2023-11-27T10:18:25Z

src/tests/simulation/src/slt.rs

+                        let next = *tokens.peek()?;
+                        if "if" == next
+                            && let Some("not") = tokens.peek().cloned()
+                            && let Some("exists") = tokens.peek().cloned() {


Oh, it's multipeek. 🥵 Confused me at the first glance.

yezizp2012

LGTM

kwannoel added 8 commits November 21, 2023 11:37

support materialized view SqlCmd

c538827

update framework for workaround

a1174e6

update deterministic recovery test

0670a16

fix

219cc47

fix test

4ba6625

fix regression

e36e00b

add support to randomly set background_ddl=true

eeabe6b

set background ddl weight to some high number

3ea4208

kwannoel added ci/run-main-cron ci/run-build-simulation ci/run-recovery-test-deterministic-simulation labels Nov 21, 2023

github-actions bot added the component/test Test related issue. label Nov 21, 2023

kwannoel changed the base branch from main to kwannoel/support-retry-ddl November 21, 2023 07:54

better trace

e058534

kwannoel mentioned this pull request Nov 21, 2023

test: add recovery test for background_ddl #13539

Closed

9 tasks

support table in creating procedure

73830ae

kwannoel force-pushed the kwannoel/support-retry-ddl-2 branch from d45042c to 73830ae Compare November 21, 2023 08:34

kwannoel added 6 commits November 21, 2023 17:30

make sure it finishes if in creating procedure

458777c

exit if i >= 5 for background_ddl

e035750

throttle kill rate

0140a28

only test kill for background mv

9321fbd

no need for separate branch

96880d0

fix

43e25e7

kwannoel force-pushed the kwannoel/support-retry-ddl-2 branch from d1e2c09 to 43e25e7 Compare November 21, 2023 11:58

kwannoel changed the base branch from kwannoel/support-retry-ddl to main November 21, 2023 12:03

kwannoel added 2 commits November 21, 2023 20:52

more trace

9b07b20

more tracing

54e9d7d

kwannoel force-pushed the kwannoel/support-retry-ddl-2 branch from 21e8b0d to 54e9d7d Compare November 21, 2023 13:17

more tracing + increase retries for bg mv

2b0391a

kwannoel added 2 commits November 22, 2023 13:55

refactor background_ddl handling in slt

bf3efc1

cleanup further

d95cc53

kwannoel changed the title ~~test: support randomly setting background_ddl to true~~ test: support background_ddl_rate in recovery test Nov 22, 2023

kwannoel marked this pull request as ready for review November 22, 2023 07:23

kwannoel requested a review from a team as a code owner November 22, 2023 07:23

kwannoel commented Nov 22, 2023

View reviewed changes

kwannoel requested review from yezizp2012, wangrunji0408 and BugenZhao November 22, 2023 07:30

stdrc reviewed Nov 24, 2023

View reviewed changes

e2e_test/background_ddl/sim/basic.slt Show resolved Hide resolved

Merge remote-tracking branch 'origin/main' into kwannoel/support-retr…

883b911

…y-ddl-2

kwannoel requested a review from chenzl25 November 27, 2023 05:37

stdrc approved these changes Nov 27, 2023

View reviewed changes

yezizp2012 reviewed Nov 27, 2023

View reviewed changes

BugenZhao reviewed Nov 27, 2023

View reviewed changes

separate background_ddl recovery fuzzing

f739575

kwannoel requested review from yezizp2012 and BugenZhao November 28, 2023 04:25

yezizp2012 approved these changes Nov 28, 2023

View reviewed changes

kwannoel added this pull request to the merge queue Nov 28, 2023

kwannoel removed this pull request from the merge queue due to a manual request Nov 28, 2023

kwannoel added this pull request to the merge queue Nov 28, 2023

Merged via the queue into main with commit 679413a Nov 28, 2023
7 of 8 checks passed

kwannoel deleted the kwannoel/support-retry-ddl-2 branch November 28, 2023 06:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: support `background_ddl_rate` in recovery test #13552

test: support `background_ddl_rate` in recovery test #13552

kwannoel commented Nov 21, 2023 •

edited

Loading

kwannoel Nov 22, 2023

kwannoel Nov 22, 2023

kwannoel Nov 22, 2023

kwannoel Nov 22, 2023

codecov bot commented Nov 22, 2023 •

edited

Loading

kwannoel commented Nov 27, 2023

stdrc left a comment

stdrc Nov 27, 2023

kwannoel Nov 27, 2023

yezizp2012 left a comment

kwannoel commented Nov 27, 2023

BugenZhao Nov 27, 2023

yezizp2012 left a comment

test: support background_ddl_rate in recovery test #13552

test: support background_ddl_rate in recovery test #13552

Conversation

kwannoel commented Nov 21, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

kwannoel Nov 22, 2023

Choose a reason for hiding this comment

kwannoel Nov 22, 2023

Choose a reason for hiding this comment

kwannoel Nov 22, 2023

Choose a reason for hiding this comment

kwannoel Nov 22, 2023

Choose a reason for hiding this comment

codecov bot commented Nov 22, 2023 • edited Loading

Codecov Report

kwannoel commented Nov 27, 2023

stdrc left a comment

Choose a reason for hiding this comment

stdrc Nov 27, 2023

Choose a reason for hiding this comment

kwannoel Nov 27, 2023

Choose a reason for hiding this comment

yezizp2012 left a comment

Choose a reason for hiding this comment

kwannoel commented Nov 27, 2023

BugenZhao Nov 27, 2023

Choose a reason for hiding this comment

yezizp2012 left a comment

Choose a reason for hiding this comment

test: support `background_ddl_rate` in recovery test #13552

test: support `background_ddl_rate` in recovery test #13552

kwannoel commented Nov 21, 2023 •

edited

Loading

codecov bot commented Nov 22, 2023 •

edited

Loading