feat(cmd_all): support override args for each node #15044

kwannoel · 2024-02-07T08:59:44Z

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

What's changed and what's your intention?

Also decouple single_node and standalone mode.

Subsequent PRs will also include profiles.

Related: #14997

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

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

src/cmd_all/src/single_node.rs

BugenZhao · 2024-02-07T09:52:13Z

src/cmd_all/src/single_node.rs

+            // This hack is required, because `update_from` treats arg[0] as the command name.
+            let prefix = "".to_string();
+            let extra_opts = iter::once(&prefix).chain(extra_opts.iter());
+            opts.update_from(extra_opts);


According to my tests, this will also overwrite all the fields with default values, if present. Will this be a problem if we're going to provide some non-default value from the profile?

Hmm what do you mean?

single_node provides some defaults, seems like that's still preserved:

https://github.com/risingwavelabs/risingwave/pull/15044/files#r1481228711

risingwave/src/cmd_all/src/single_node.rs

Line 187 in 482ded4

fn default_frontend_opts() -> FrontendOpts {

The defaults I'm referring to are here.

By "default value" I'm not referring to fn default_xx_opts, but the fields attributed with #[clap(default_value = "..")].

Would you mind trying...

--meta-addr 127.0.0.1:8888 --meta-extra-opts=--advertise-addr 127.0.0.1:9999

to see if 8888 gets preserved?

Oh in this case I expect it to be overridden.

I'm not sure whether I have made clear, but here I intentionally overwrite the advertise address instead of the listen address. They are different options.

To make it more clear, you can try passing an empty string as the extra options. The 8888 could still be overwritten. Is this also expected?

Added it: 62c2310

Oh yeah you're right... 8888 is not preserved in the listen-addr:

listen_addr: "127.0.0.1:5690",

I guess this PR is blocked here now.

Yes. I don't have any other ideas for now. Probably have to make a PR to clap to try and make a case for this.

Otherwise, I think we have to generate a node options builder per node, and use that to update the base options per node.

kwannoel · 2024-02-07T10:06:39Z

src/cmd_all/src/single_node.rs

+                            privatelink_endpoint_default_tags: None,
+                            config_path: "",
+                            backend: Some(
+                                Sql,


The initial options we provided with Sql backend is preserved and not overridden from providing meta opts.

kwannoel · 2024-02-07T10:15:40Z

src/cmd_all/src/single_node.rs

+                            vpc_id: None,
+                            security_group_id: None,
+                            listen_addr: "127.0.0.1:5690",
+                            advertise_addr: "127.0.0.1:9999",


Advertise address is overridden as expected.

kwannoel · 2024-02-07T10:15:56Z

src/cmd_all/src/single_node.rs

+                            state_store: Some(
+                                "hummock+fs:///root/.risingwave/state_store",
+                            ),
+                            data_directory: Some(


Data directory also overridden

xxchan · 2024-02-07T10:17:22Z

src/cmd_all/src/single_node.rs

+
+    /// Extra options for meta node.
+    #[clap(long, env = "RW_SINGLE_NODE_META_EXTRA_OPTS")]
+    meta_extra_opts: Option<String>,
+
+    /// Extra options for compute node.
+    #[clap(long, env = "RW_SINGLE_NODE_COMPUTE_EXTRA_OPTS")]
+    compute_extra_opts: Option<String>,
+
+    /// Extra options for frontend node.
+    #[clap(long, env = "RW_SINGLE_NODE_FRONTEND_EXTRA_OPTS")]
+    frontend_extra_opts: Option<String>,
+
+    /// Extra options for compactor node.
+    #[clap(long, env = "RW_SINGLE_NODE_COMPACTOR_EXTRA_OPTS")]
+    compactor_extra_opts: Option<String>,


So we are adding standalone style opts to single_node mode now. This makes me think what about adding single_node style map_opts to standalone mode.. #14997 (comment)

(Just random thoughts. Both are OK to me though

We will deprecate standalone mode eventually, as per #14997

kwannoel · 2024-02-07T10:18:39Z

src/cmd_all/src/single_node.rs

+                    ),
+                    frontend_opts: Some(
+                        FrontendOpts {
+                            listen_addr: "0.0.0.0:4566",


Default value we specified in src/frontend/src/lib.rs is 127.0.0.1:4566.

But in standalone mode, we specify 0.0.0.0:4566. So the node level defaults do not override as well.

src/cmd_all/src/single_node.rs

xxchan · 2024-02-07T10:33:22Z

src/cmd_all/src/single_node.rs

+--meta-extra-opts=--advertise-addr 127.0.0.1:9999 --data-directory \"some path with spaces\" \
+--state-store hummock+fs:///root/.risingwave/state_store \
+--sql-endpoint sqlite:///root/.risingwave/meta_store/single_node.db?mode=rwc
+--compute-extra-opts=--listen-addr 127.0.0.1:8888 --total-memory-bytes 123 --parallelism 10


Is this correct? Shouldn't we quote the value of --meta-extra-opts=?

I have seen this before. Also curious why it works 😄

xxchan

Generally LGTM if we make sure the details are correct. Cool!

fuyufjh · 2024-02-19T09:53:04Z

src/cmd_all/src/single_node.rs

+            // This hack is required, because `update_from` treats arg[0] as the command name.
+            let prefix = "".to_string();
+            let extra_opts = iter::once(&prefix).chain(extra_opts.iter());
+            opts.update_from(extra_opts);


I guess this PR is blocked here now.

fuyufjh · 2024-02-19T09:57:52Z

src/cmd_all/src/single_node.rs

@@ -53,6 +56,8 @@ pub static DEFAULT_SINGLE_NODE_STATE_STORE_URL: LazyLock<String> = LazyLock::new
    about = "[default] The Single Node mode. Start all services in one process, with process-level options. This will be executed if no subcommand is specified"
 )]
 /// Here we define our own defaults for the single node mode.
+// TODO(kwannoel): Support user supplied profiles.
+// Perhaps https://docs.rs/clap-serde-derive/0.2.1/clap_serde_derive/?


I think it's not that necessary. We have lots of configurations in YAML, and they are categorized into storage, compute, server, etc. to make it readable, but we don't want all of them to be settable from CLI.

Co-authored-by: Bugen Zhao <[email protected]>

kwannoel · 2024-03-11T05:23:12Z

Look at #15577 instead.

kwannoel requested a review from BugenZhao February 7, 2024 08:59

github-actions bot added the type/feature label Feb 7, 2024

kwannoel requested review from fuyufjh, xxchan and lmatz February 7, 2024 09:00

kwannoel added the need-cherry-pick-release-1.7 label Feb 7, 2024

BugenZhao reviewed Feb 7, 2024

View reviewed changes

kwannoel commented Feb 7, 2024

View reviewed changes

xxchan reviewed Feb 7, 2024

View reviewed changes

kwannoel commented Feb 7, 2024

View reviewed changes

xxchan reviewed Feb 7, 2024

View reviewed changes

src/cmd_all/src/single_node.rs Outdated Show resolved Hide resolved

xxchan reviewed Feb 7, 2024

View reviewed changes

fuyufjh reviewed Feb 19, 2024

View reviewed changes

kwannoel removed the need-cherry-pick-release-1.7 label Feb 28, 2024

kwannoel and others added 13 commits March 6, 2024 16:33

support meta node extra opts

9881566

decouple standalone and single node opts

fa96bc4

add test + hack for parsing extra opts

7ad51ad

support extra opts for other nodes

1ee1281

fix

ca20a45

fix

d4d9253

fix the dynamic state store

3b81647

Update src/cmd_all/src/single_node.rs

91a686f

Co-authored-by: Bugen Zhao <[email protected]>

fix other service logs

d268f12

fix dynamic sqlite endpoint as well

7683f8c

use normalize instead

7cec0ef

support opts at top level

8416f8d

add test

0142bdc

kwannoel force-pushed the kwannoel/provide-extra-opts branch from 62c2310 to 0142bdc Compare March 6, 2024 08:53

kwannoel closed this Mar 11, 2024

xxchan mentioned this pull request Mar 11, 2024

feat(standalone): add extra opts to single node #15577

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cmd_all): support override args for each node #15044

feat(cmd_all): support override args for each node #15044

kwannoel commented Feb 7, 2024 •

edited

Loading

BugenZhao Feb 7, 2024

kwannoel Feb 7, 2024

kwannoel Feb 7, 2024 •

edited

Loading

BugenZhao Feb 7, 2024

kwannoel Feb 7, 2024

BugenZhao Feb 7, 2024

kwannoel Feb 7, 2024

kwannoel Feb 7, 2024

fuyufjh Feb 19, 2024

kwannoel Feb 20, 2024

kwannoel Feb 7, 2024

kwannoel Feb 7, 2024

kwannoel Feb 7, 2024

xxchan Feb 7, 2024

kwannoel Feb 7, 2024

kwannoel Feb 7, 2024

xxchan Feb 7, 2024

fuyufjh Feb 19, 2024 •

edited

Loading

xxchan left a comment

fuyufjh Feb 19, 2024

fuyufjh Feb 19, 2024

kwannoel commented Mar 11, 2024

feat(cmd_all): support override args for each node #15044

feat(cmd_all): support override args for each node #15044

Conversation

kwannoel commented Feb 7, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwannoel Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuyufjh Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

xxchan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwannoel commented Mar 11, 2024

kwannoel commented Feb 7, 2024 •

edited

Loading

kwannoel Feb 7, 2024 •

edited

Loading

fuyufjh Feb 19, 2024 •

edited

Loading