Skip to content
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.

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

feat: try to enable recovery test in ci #6347

Merged
merged 10 commits into from
Nov 16, 2022
Merged

feat: try to enable recovery test in ci #6347

merged 10 commits into from
Nov 16, 2022

Conversation

yezizp2012
Copy link
Member

@yezizp2012 yezizp2012 commented Nov 14, 2022

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

As title.

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

If your pull request contains user-facing changes, please specify the types of the changes, and create a release note. Otherwise, please feel free to remove this section.

Types of user-facing changes

Please keep the types that apply to your changes, and remove those that do not apply.

  • Installation and deployment
  • Connector (sources & sinks)
  • SQL commands, functions, and operators
  • RisingWave cluster configuration changes
  • Other (please specify in the release note below)

Release note

Please create a release note for your changes. In the release note, focus on the impact on users, and mention the environment or conditions where the impact may occur.

Refer to a related PR or issue link (optional)

@codecov
Copy link

codecov bot commented Nov 15, 2022

Codecov Report

Merging #6347 (a1e1a59) into main (ddae931) will decrease coverage by 0.23%.
The diff coverage is 28.12%.

@@            Coverage Diff             @@
##             main    #6347      +/-   ##
==========================================
- Coverage   74.28%   74.05%   -0.24%     
==========================================
  Files         960      966       +6     
  Lines      156525   157225     +700     
==========================================
+ Hits       116277   116434     +157     
- Misses      40248    40791     +543     
Flag Coverage Δ
rust 74.05% <28.12%> (-0.24%) ⬇️

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

Impacted Files Coverage Δ
src/common/src/types/chrono_wrapper.rs 46.66% <0.00%> (-15.91%) ⬇️
src/common/src/types/interval.rs 72.10% <0.00%> (-7.47%) ⬇️
src/common/src/types/mod.rs 72.40% <0.00%> (ø)
src/expr/src/expr/build_expr_from_prost.rs 53.76% <0.00%> (-1.91%) ⬇️
src/expr/src/expr/expr_binary_nonnull.rs 62.80% <0.00%> (-8.89%) ⬇️
src/expr/src/expr/mod.rs 47.91% <0.00%> (-1.02%) ⬇️
src/expr/src/vector_op/date_trunc.rs 0.00% <0.00%> (ø)
src/frontend/src/observer/observer_manager.rs 0.00% <0.00%> (ø)
src/frontend/src/optimizer/plan_node/stream.rs 13.30% <ø> (ø)
src/meta/src/manager/catalog/fragment.rs 32.61% <0.00%> (+0.25%) ⬆️
... and 50 more

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

@yezizp2012 yezizp2012 marked this pull request as ready for review November 15, 2022 08:54
@yezizp2012
Copy link
Member Author

As there is still a large time gap between CI and local deterministic recovery tests, in this PR, the deterministic recovery test will ignore the two tests: tpch_snapshot.slt and tpch_upstream.slt, in which the executors are almost included already in the other tests in streaming.

Let's enable recovery test in CI firstly to prevent any further potential bugs in the future PRs.

@BugenZhao
Copy link
Member

The per-step time usage seems weird. 🤔

image

@yezizp2012
Copy link
Member Author

The per-step time usage seems weird. 🤔

image

I believe the time cost and the title are just one line apart. The 5m7s should be the streaming's cost time. 😅

Copy link
Contributor

@wangrunji0408 wangrunji0408 left a comment

Choose a reason for hiding this comment

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

The test part LGTM!

src/tests/simulation/src/main.rs Show resolved Hide resolved
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. Long expected!!

I guess we're not utilizing the multicore in simulation e2e tests. Is this a possible approach to speed things up? cc @wangrunji0408

@mergify mergify bot merged commit ceb2f1e into main Nov 16, 2022
@mergify mergify bot deleted the feat/enable-ci-recovery branch November 16, 2022 07:19
@wangrunji0408
Copy link
Contributor

I guess we're not utilizing the multicore in simulation e2e tests. Is this a possible approach to speed things up?

Yeah. It is possible but is also hard to be done while keeping determinism. (see the explanation here)
So I don't plan to parallelize it now, but I will keep this topic open. Maybe some new ideas will come up in the future.

@BugenZhao
Copy link
Member

What about simply parallelizing the tests to different simulated clusters at the granularity of slt files? 🤔

@wangrunji0408
Copy link
Contributor

What about simply parallelizing the tests to different simulated clusters at the granularity of slt files? 🤔

Definately that is feasible!

xxchan pushed a commit that referenced this pull request Nov 19, 2022
* feat: try to enable recovery test in ci

* fix

* fix init notification

* run simulation in release

* some fix and temporarily disable tpch test in recovery

* fix sink recovery in frontend

* config kill-rate

* fix env

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants