feat: add loom test for the counter

[BowenXiao1999]

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

What's changed and what's your intention?

Basically same as #6822. But we reuse the Counter to avoid code duplicate. Copy the content here.

Use https://github.com/tokio-rs/loom for concurrency test. Maybe can make the correctness more confident. But I'm not so sure how to write the best test.

it requires some code change for previous structure, so may need discuss:

remove workspacke hack in task_stats_alloc crate. Otherwise there will be package conflict if you run RUSTFLAGS="--cfg loom" cargo test --test loom. Have not investigate deeply but simple remove just works.

Refactor &'static AtomicUsize to be NonNull. This is because the AtomicUsize in loom type do not support .as_mut_ptr (described in What to do when loom::AtomicUsize do not implement as_mut_ptr() tokio-rs/loom#298), so we use NonNull as intermediate workaround, that will add some unsafe code in .add() and .sub(). But seems OK.

To test the drop, I have to add a flag var AtomicUsize in .sub() to ensure that the value is dropped. Please provide some suggestions if you have better ideas on how to write test.

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)

[github-actions]

license-eye has totally checked 2540 files.

Valid	Invalid	Ignored	Fixed
1205	1	1334	0

Click to see the invalid file list

• src/utils/task_stats_alloc/tests/loom.rs

[BowenXiao1999]

Add this due to the compiler

[BugenZhao]

remove workspacke hack in task_stats_alloc crate. Otherwise there will be package conflict if you run RUSTFLAGS="--cfg loom" cargo test --test loom. Have not investigate deeply but simple remove just works.

I think we don't need to depend on workspace_hack for all of these utility crates: it's for the case where different crates in the workspace require different sets of features of a dependency, so that compiling a crate separately (like running tests under risingwave_stream) will lead to a recompilation even we've compiled the whole workspace. We rarely compile these utilities separately, so there seems no need to add this, even for not(loom). 🤔

[BugenZhao]

Refactor &'static AtomicUsize to be NonNull.

LGTM! It's actually not in a static lifetime. To claim that the lifetime is manually managed, using a raw pointer sounds better.

[BugenZhao]

Good work!

[wangrunji0408]

LGTM!

[BugenZhao]

Rest LGTM

[liurenjie1024]

Generally LGTM

[BugenZhao]

🚀

[BowenXiao1999]

Given that this utils (specifically the Counter) should not be frequently changed, I decided not to add this test into CI, seems like it will doubles the runnig time or more (Re-compile).

Have added comments that remind people to re-run this test locally and future TODOs.

[liurenjie1024]

LGTM

[BugenZhao]

I believe the timeout is caused by sqlsmith due to .sh file changed. Compile this utility don't cost too much. Anyway, agree on not adding to CI.

[liurenjie1024]

How about adding it to daily ci?

[BowenXiao1999]

How about adding it to daily ci?

I'm not sure where is our daily ci. Do you mean the main-cron.yml? But it seems like we still need to create a new label job (do not put it the same scripts with unit test, otherwise there is still timeout)

Let me try put the test behind the sqlsmith

[codecov]

Codecov Report

Merging #6888 (3af01ba) into main (d2dfad3) will increase coverage by 0.00%.
The diff coverage is 87.50%.

@@           Coverage Diff           @@
##             main    #6888   +/-   ##
=======================================
  Coverage   73.12%   73.12%           
=======================================
  Files        1037     1037           
  Lines      165781   165785    +4     
=======================================
+ Hits       121231   121234    +3     
- Misses      44550    44551    +1     

Flag	Coverage Δ
rust	73.12% <87.50%> (+<0.01%)	⬆️

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

Impacted Files	Coverage Δ
src/utils/task_stats_alloc/src/lib.rs	82.40% <87.50%> (-0.25%)	⬇️

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

[mergify]

Hey @BowenXiao1999, this pull request failed to merge and has been dequeued from the merge train. If you believe your PR failed in the merge train because of a flaky test, requeue it by clicking "Update branch" or pushing an empty commit with git commit --allow-empty -m "rerun" && git push.