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(meta): improve param updating process when the cluster version upgrades #8260

Merged
merged 9 commits into from
Mar 3, 2023

Conversation

Gun9niR
Copy link
Contributor

@Gun9niR Gun9niR commented Mar 1, 2023

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

What's changed and what's your intention?

When the RW cluster upgrades to a new version, new parameters may be introduced. They may either be:

  • Entirely new params that need to be initialized
  • Or newer versions of existing parameters that need to retain the same effect on the cluster.

For the first case, SystemParamManager will detect missing params and initialize them with CLI values or default values.

For the second case, a framework is added to conveniently derive the value of new params with existing params. The developer simply needs to override the trait OverrideFromParams to define custom rules.

Checklist For Contributors

  • 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)

Checklist For Reviewers

  • I have requested macro/micro-benchmarks as this PR can affect performance substantially, and the results are shown.

Documentation

  • My PR DOES NOT contain user-facing changes.
Click here for Documentation

Types of user-facing changes

Please keep the types that apply to your changes, and remove the others.

  • 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

@codecov
Copy link

codecov bot commented Mar 1, 2023

Codecov Report

Merging #8260 (b219cf1) into main (b237d7d) will decrease coverage by 0.01%.
The diff coverage is 55.26%.

@@            Coverage Diff             @@
##             main    #8260      +/-   ##
==========================================
- Coverage   71.57%   71.57%   -0.01%     
==========================================
  Files        1130     1130              
  Lines      184096   184098       +2     
==========================================
- Hits       131771   131766       -5     
- Misses      52325    52332       +7     
Flag Coverage Δ
rust 71.57% <55.26%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
src/meta/src/manager/system_param/model.rs 91.42% <ø> (ø)
src/meta/src/manager/system_param/mod.rs 33.33% <7.14%> (-2.67%) ⬇️
src/common/src/system_param/mod.rs 94.11% <83.33%> (-3.41%) ⬇️
src/storage/src/hummock/sstable/block.rs 93.90% <0.00%> (-1.83%) ⬇️
src/meta/src/hummock/mock_hummock_meta_client.rs 64.43% <0.00%> (-1.04%) ⬇️
src/object_store/src/object/mod.rs 48.07% <0.00%> (-0.41%) ⬇️
src/storage/src/hummock/compactor/mod.rs 80.19% <0.00%> (-0.20%) ⬇️
src/common/src/cache.rs 97.15% <0.00%> (-0.11%) ⬇️
src/meta/src/hummock/manager/mod.rs 78.83% <0.00%> (-0.07%) ⬇️
... and 3 more

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

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.

Rest LGTM.

Comment on lines +55 to +56
$macro /* Define future deprecated params here, such as
* ,{ backup_storage_directory, String, "backup".to_string() } */
Copy link
Member

Choose a reason for hiding this comment

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

This really works but seems a little bit strange to pass deprecated params to the macro of undeprecated_params. 🤣

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really can't think of another way to put two for_all_xxx mcros together 😢

@Gun9niR Gun9niR force-pushed the zhidong/system-params-refine branch from f81000a to 3c8ded6 Compare March 3, 2023 06:24
@Gun9niR Gun9niR mentioned this pull request Mar 3, 2023
9 tasks
@mergify
Copy link
Contributor

mergify bot commented Mar 3, 2023

Hey @Gun9niR, 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.

@Gun9niR Gun9niR added this pull request to the merge queue Mar 3, 2023
Merged via the queue into main with commit 870cbf7 Mar 3, 2023
@Gun9niR Gun9niR deleted the zhidong/system-params-refine branch March 3, 2023 08:22
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.

2 participants