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: enable shared source in session variable by default, and add cluster-level config to disable #18749

Merged
merged 3 commits into from
Oct 21, 2024

Conversation

xxchan
Copy link
Member

@xxchan xxchan commented Sep 28, 2024

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

What's changed and what's your intention?

User interface

  • Can use session variable streaming_use_shared_source to control whether to use shared feature:
# change the config in the current session, default true
SET streaming_use_shared_source=[true|false];

# change the default value of the session variable in the cluster
# (the current session is not affected)
ALTER SYSTEM SET streaming_use_shared_source=[true|false];
  • There’s also a cluster config stream_enable_shared_source ([streaming.developer] in risingwave.toml). Set it to false can completely disable this feature in the cluster.

What's changed

The config is similar to streaming_use_arrangement_backfill (session) and stream_enable_arrangement_backfill (cluster)

For more discussions see https://risingwave-labs.slack.com/archives/C06AL46R10S/p1729051215981299

  • Rename session var enable_shared_source to streaming_use_shared_source, to be consistent with streaming_use_arrangement_backfill
  • Make the session var defaults to true
  • Add config stream_enable_shared_source in [streaming.developer] in risingwave.toml, defaults to true. When this is false, the feature is disabled in the cluster (to re-enable, need to update toml and restart); When it's true, respect session variable.
  • Changes to tests:
    • testing using ALTER SOURCE need to use non-shared source, as altering shared source is not supported yet
    • rate_limit_source_kafka.slt: alter source_rate_limit do not affect source backfill.
  • Tracking issue for shared source #16003

Benchmarks

Discussed with @lmatz , we will first disable shared source in daily benchmark pipeline. After fixing things like metrics collection, we add a new weekly benchmark for shared source.

We will need to tweak benchmark pipelines after enabling shared source, otherwise they may fail. Click to see details

Nexmark

Nexmark first creates a Kafka topic and fill data, then builds an MV. This will result in the entire benchmark process being backfill.

  1. there is a slight change in metrics; it doesn't have source throughput, but only source backfill throughput, so it will fail.
  2. the performance during backfill might be worse than the source

We can see backfill performance is indeed worse than source:

image image

Longevity

It didn't fail, but the performance behavior will look different.

We have enable_create_mv_nexmark_table = true by default. So we have 3 source executors (bid/auction/person are MVs) -> 1 source executor + 3 backfill executors.

Previously the 3 sources have individual consumption progress, but now they share consumption progress. This is like "unified topic" in nexmark benchmark.

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

Need release note & doc for the whole shared source feature: https://www.notion.so/risingwave-labs/Doc-for-Shared-Source-10ef0d69cb768078b2a5e05bfa5d4807

Copy link
Member Author

xxchan commented Sep 28, 2024

@xxchan xxchan marked this pull request as ready for review September 28, 2024 12:31
@xxchan xxchan force-pushed the xxchan/share-source-config branch from ea1e46a to b6a1d39 Compare September 28, 2024 15:07
@xxchan xxchan changed the base branch from main to 09-27-refactor_unify_duplicated_alter___name_methods September 28, 2024 15:07
@xxchan xxchan force-pushed the 09-27-refactor_unify_duplicated_alter___name_methods branch from 9dbf61c to fe849f7 Compare September 29, 2024 02:25
@xxchan xxchan force-pushed the xxchan/share-source-config branch from b6a1d39 to 91524d3 Compare September 29, 2024 02:26
@xxchan xxchan changed the base branch from 09-27-refactor_unify_duplicated_alter___name_methods to main September 29, 2024 02:26
@BugenZhao
Copy link
Member

Currently session variables have different styles:

#13265

@xxchan xxchan changed the title feat: enable shared source in session variable by default, and add system variable to disable feat: enable shared source in session variable by default, and add cluster-level config to disable Sep 30, 2024
@xxchan xxchan force-pushed the xxchan/share-source-config branch 2 times, most recently from c80cbf3 to 7a29ddb Compare October 11, 2024 07:42
@xxchan xxchan changed the base branch from main to xxchan/move-test October 11, 2024 07:42
@xxchan xxchan force-pushed the xxchan/share-source-config branch from 357a0c6 to 25cadfa Compare October 11, 2024 08:31
@xxchan xxchan force-pushed the xxchan/share-source-config branch from 25cadfa to 8579768 Compare October 11, 2024 08:47
Base automatically changed from xxchan/move-test to main October 11, 2024 11:43
@xxchan xxchan force-pushed the xxchan/share-source-config branch from 8579768 to 37bd4ce Compare October 14, 2024 12:54
@xxchan xxchan changed the base branch from main to 10-14-refactor_test_move_some_tests_from_source_legacy_to_source_inline October 14, 2024 12:54
@xxchan xxchan force-pushed the 10-14-refactor_test_move_some_tests_from_source_legacy_to_source_inline branch from 660fa6c to eaebb9c Compare October 14, 2024 13:50
@xxchan xxchan force-pushed the xxchan/share-source-config branch from 37bd4ce to 03875c2 Compare October 14, 2024 13:50
@xxchan xxchan force-pushed the 10-14-refactor_test_move_some_tests_from_source_legacy_to_source_inline branch from eaebb9c to 1104609 Compare October 14, 2024 14:18
Base automatically changed from 10-15-feat_support_background_ddl_for_mv_on_shared_source to main October 17, 2024 06:34
@xxchan xxchan force-pushed the xxchan/share-source-config branch from 899a05f to 7326f9d Compare October 17, 2024 06:52
Copy link
Member

@fuyufjh fuyufjh left a comment

Choose a reason for hiding this comment

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

LGTM.

Shall we cherry-pick to release 2.1?

/// If false, the shared source will be disabled,
/// even if session variable set.
/// If true, it's decided by session variable `streaming_use_shared_source` (default true)
pub enable_shared_source: bool,
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't follow the standard pattern of a session/global variable, particularly, the "cluster-level config" should be SystemParams instead of RwConfig. However, as the configuration is not user-facing but only for us, it's acceptable to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

standard pattern of a session/global variable

FYI actually session variable can also be ALTER SYSTEM to be changed globally.

Just add this extra gate in case it's needed.

@xxchan
Copy link
Member Author

xxchan commented Oct 17, 2024

Shall we cherry-pick to release 2.1?

Yes, it's planned

@xxchan
Copy link
Member Author

xxchan commented Oct 17, 2024

Will merge this after changing benchmark pipeline https://linear.app/risingwave-labs/issue/PERF-163

@xxchan xxchan force-pushed the xxchan/share-source-config branch from 7326f9d to 2bf03cd Compare October 18, 2024 00:49
…stem variable to disable

The config is similar to `streaming_use_arrangement_backfill` (session) and `stream_enable_arrangement_backfill` (system)

BTW one problem found: Currently session variables have different styles:
- with rw prefix: `rw_streaming_enable_delta_join`, `rw_batch_enable_sort_agg`, `rw_enable_share_plan`
- without rw prefix: `streaming_use_arrangement_backfill`, `batch_enable_distributed_dml`

Signed-off-by: xxchan <[email protected]>
@xxchan xxchan force-pushed the xxchan/share-source-config branch from 2bf03cd to 06bd105 Compare October 21, 2024 01:40
@xxchan xxchan enabled auto-merge October 21, 2024 01:41
@xxchan xxchan added this pull request to the merge queue Oct 21, 2024
Merged via the queue into main with commit 3c57ef8 Oct 21, 2024
31 of 32 checks passed
@xxchan xxchan deleted the xxchan/share-source-config branch October 21, 2024 02:34
github-actions bot pushed a commit that referenced this pull request Oct 21, 2024
github-merge-queue bot pushed a commit that referenced this pull request Oct 21, 2024
…uster-level config to disable (#18749) (#19023)

Signed-off-by: xxchan <[email protected]>
Co-authored-by: xxchan <[email protected]>
@lmatz lmatz added the user-facing-changes Contains changes that are visible to users label Oct 28, 2024
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.

4 participants