Skip to content

fix: set default barrier interval to 1s #6553

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Nov 25, 2022
Merged

fix: set default barrier interval to 1s #6553

merged 5 commits into from
Nov 25, 2022

Conversation

@fuyufjh
Copy link
Collaborator

@fuyufjh fuyufjh commented Nov 24, 2022 •
edited
Loading

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

What's changed and what's your intention?

Set the default barrier interval to 1s, which is used by our cloud service.

Meanwhile, keep the testing config as 250ms.

The config in risingwave.toml was not changed, so I suppose our e2e test will also be run in 250ms interval.

Checklist

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

Documentation

It seems that we didn't mention anything about checkpointing in docs...

Refer to a related PR or issue link (optional)

None

@github-actions github-actions bot added the type/fix Type: Bug fix. Only for pull requests. label Nov 24, 2022
@BugenZhao
Copy link
Member

BugenZhao commented Nov 24, 2022

The config in risedev.yml was not changed

Do you mean risingwave.toml? 👀

@fuyufjh
Copy link
Collaborator Author

fuyufjh commented Nov 24, 2022

The config in risedev.yml was not changed

Do you mean risingwave.toml? 👀

Yes 🤣

@BugenZhao
Copy link
Member

BugenZhao commented Nov 24, 2022

It looks like the scaling simulation test requires a more frequent barrier interval, while it doesn't follow the config in risingwave.toml... 🤔 cc @wangrunji0408

@wangrunji0408
Copy link
Contributor

wangrunji0408 commented Nov 24, 2022

It looks like the scaling simulation test requires a more frequent barrier interval, while it doesn't follow the config in risingwave.toml... 🤔 cc @wangrunji0408

Yes, it doesn't use the config from risingwave.toml because it has a larger parallelism setting which makes the simulation very slow. 😰

@fuyufjh
Copy link
Collaborator Author

fuyufjh commented Nov 24, 2022

It looks like the scaling simulation test requires a more frequent barrier interval, while it doesn't follow the config in risingwave.toml... 🤔 cc @wangrunji0408

Yes, it doesn't use the config from risingwave.toml because it has a larger parallelism setting which makes the simulation very slow. 😰

Is it possible to inject the config via file in simulation test?

risingwave/src/tests/simulation_scale/src/cluster.rs

Lines 80 to 86 in baddd63

let opts = risingwave_meta::MetaNodeOpts::parse_from([
"meta-node",
"--listen-addr",
"0.0.0.0:5690",
"--backend",
"mem",
]);

@BugenZhao
Copy link
Member

BugenZhao commented Nov 24, 2022

The recovery test only uses 3m54s under this configuration. 🤯 cc @yezizp2012

@yezizp2012 yezizp2012 changed the title fix: set defeault barrier interval to 1s fix: set default barrier interval to 1s Nov 24, 2022
@BugenZhao
add temp config file
08411f8 
Signed-off-by: Bugen Zhao <[email protected]>
@BugenZhao
use static lazy lock
acaf364 
Signed-off-by: Bugen Zhao <[email protected]>
@BugenZhao BugenZhao force-pushed the eric/adjust_barrier_interval branch from 0470647 to acaf364 Compare November 24, 2022 07:36
@yezizp2012
Copy link
Member

yezizp2012 commented Nov 24, 2022 •
edited
Loading

The recovery test only uses 3m54s under this configuration. 🤯 cc @yezizp2012

I guess by changing barrier interval from 250ms to 1s, barrier-related simulations will be 3/4 less. This could be the reason for the reduction in total time. Does it make sense ? @wangrunji0408 🤔

@wangrunji0408
Copy link
Contributor

wangrunji0408 commented Nov 24, 2022 •
edited
Loading

The recovery test only uses 3m54s under this configuration. 🤯 cc @yezizp2012

I guess by changing barrier interval from 250ms to 1s, barrier-related simulations will be 3/4 less. Does it make sense ? @wangrunji0408 🤔

Make sense! In fact all simulations may benefit. 🥵

BugenZhao
BugenZhao reviewed Nov 24, 2022
View reviewed changes
src/common/src/config.rs

pub fn barrier_interval_ms() -> u32 {
250
1000
Copy link
Member

@BugenZhao BugenZhao Nov 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we note somewhere (in toml?) that the default value without configuration file here differs from the default value in risingwave.toml? 🥵

BugenZhao
BugenZhao approved these changes Nov 24, 2022
View reviewed changes
Copy link
Member

@BugenZhao BugenZhao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I've added a separate configuration file for the simulation scaling test and hope it works.

@codecov
Copy link

codecov bot commented Nov 24, 2022 •
edited
Loading

Codecov Report

Merging #6553 (f5eb8ba) into main (a07f267) will decrease coverage by 0.04%.
The diff coverage is 55.13%.

@@            Coverage Diff             @@
##             main    #6553      +/-   ##
==========================================
- Coverage   73.86%   73.81%   -0.05%     
==========================================
  Files        1002     1003       +1     
  Lines      162583   162848     +265     
==========================================
+ Hits       120088   120205     +117     
- Misses      42495    42643     +148
Flag Coverage Δ
rust 73.81% <55.13%> (-0.05%) ⬇️

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

Impacted Files Coverage Δ
src/common/src/config.rs 69.71% <0.00%> (ø)
src/source/src/parser/protobuf/parser.rs 54.16% <0.00%> (ø)
src/storage/src/lib.rs 100.00% <ø> (ø)
src/storage/src/store_impl.rs 9.58% <0.00%> (-4.23%) ⬇️
src/source/src/parser/canal/simd_json_parser.rs 62.80% <67.60%> (+7.07%) ⬆️
src/source/src/parser/canal/util.rs 76.19% <76.19%> (ø)
...c/frontend/src/optimizer/plan_node/logical_join.rs 93.32% <100.00%> (+0.81%) ⬆️
...c/frontend/src/optimizer/plan_node/logical_scan.rs 95.90% <100.00%> (ø)
src/meta/src/hummock/compaction_scheduler.rs 82.38% <100.00%> (+0.19%) ⬆️
src/meta/src/manager/env.rs 95.61% <100.00%> (-0.36%) ⬇️
... and 12 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@yezizp2012
Copy link
Member

yezizp2012 commented Nov 24, 2022

The recovery test only uses 3m54s under this configuration. 🤯 cc @yezizp2012

I guess by changing barrier interval from 250ms to 1s, barrier-related simulations will be 3/4 less. Does it make sense ? @wangrunji0408 🤔

Make sense! In fact all simulations may benefit. 🥵

The end-to-end-test-deterministic-simulation also reduced from 9min+ to 7min+. 🥵

@mergify mergify bot merged commit 1c3cce7 into main Nov 25, 2022
@mergify mergify bot deleted the eric/adjust_barrier_interval branch November 25, 2022 16:04
@BugenZhao BugenZhao mentioned this pull request Dec 1, 2022
Merged
3 tasks
@fuyufjh fuyufjh mentioned this pull request Dec 7, 2022
Merged
3 tasks
@xxchan xxchan mentioned this pull request Dec 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hzxa21 hzxa21 hzxa21 approved these changes

@yezizp2012 yezizp2012 yezizp2012 approved these changes

@BugenZhao BugenZhao BugenZhao approved these changes

@Little-Wallace Little-Wallace Awaiting requested review from Little-Wallace

@xxhZs xxhZs Awaiting requested review from xxhZs

Assignees

No one assigned

Labels

type/fix Type: Bug fix. Only for pull requests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@fuyufjh @BugenZhao @wangrunji0408 @yezizp2012 @hzxa21 @skyzh