[1/n][Release] Renames and configuration updates #2566

AhmedSoliman · 2025-01-28T13:01:08Z

Summary:
This renames a few configuration and command line arguments while keeping compatibility with old options. This also introduces extra documentation on those configuration keys

allow-bootstrap -> auto-provision
bootstrap-num-partitions -> default-num-partitions (aligns with default-partition-replication)
[bifrost.replicated-loglet] default_replication-property -> default_log_replication

Stack created with Sapling. Best reviewed with ReviewStack.

github-actions · 2025-01-28T13:32:06Z

Test Results

7 files 7 suites 4m 32s ⏱️
47 tests 46 ✅ 1 💤 0 ❌
182 runs 179 ✅ 3 💤 0 ❌

Results for commit 97ad3f2.

♻️ This comment has been updated with latest results.

tillrohrmann

LGTM. +1 for merging :-)

tillrohrmann · 2025-01-28T16:22:39Z

crates/types/src/config/cli_option_overrides.rs

+    /// Auto Cluster Provisioning
+    ///
+    /// If true, then this node is allowed to automatically provision as a new cluster.
+    /// This node *must* have an admin role and a new nodes configuration will be created that includes this node.


I think it is no longer required that the node that provisions the cluster runs the admin role.

tillrohrmann · 2025-01-28T16:23:55Z

crates/types/src/config/common.rs

    ///
-    /// Bootstrap is allowed by default, unless it is explicitly disabled in config files.
+    /// If true, then this node is allowed to automatically provision as a new cluster.
+    /// This node *must* have an admin role and a new nodes configuration will be created that includes this node.


Same comment as above.

tillrohrmann · 2025-01-28T16:25:07Z

crates/types/src/nodes_config.rs

@@ -51,7 +51,7 @@ pub enum Role {
    // todo switch to serializing as "metadata-server" in version 1.3
    #[serde(rename(serialize = "metadata-store"))]
    MetadataServer,
-    /// [IN DEVELOPMENT] Serves a log server for replicated loglets
+    /// [PREVIEW FEATURE] Serves a log-server for replicated loglets


[__PREVIEW FEATURE__]?

tillrohrmann · 2025-01-28T17:05:18Z

crates/types/src/config/common.rs

    ///
    /// Default: true
-    pub allow_bootstrap: bool,
+    #[serde(alias = "allow-bootstrap")]


The alias might actually result into duplicate fields when trying to read a configuration where allow-bootstrap is defined.

Ugh, I couldn't actually find a good solution to this :( without investing too much time, and I don't want us to give up on the rename.

I'm not sure if there are good ideas here :(

The way I solved it in another PR is by introducing a Shadow variant: https://github.com/restatedev/restate/pull/2571/files#diff-b07de33428bbafda6c26f429c0f36178f5ef8e6215e7a1f187c72142417ecc29R249. Not particularly proud of it, though.

crates/types/src/config/cli_option_overrides.rs

crates/types/src/config/admin.rs

crates/types/src/config/bifrost.rs

pcholakov · 2025-01-29T14:11:13Z

crates/types/src/config/cli_option_overrides.rs

-    pub auto_provision_partitions: Option<bool>,
+    /// Default: 24
+    #[clap(long, global = true, alias = "bootstrap-num-partitions")]
+    pub default_num_partitions: Option<NonZeroU64>,


~~This breaks in a weird way when set via an env variable:~~

❯ RESTATE_BOOTSTRAP_NUM_PARTITIONS=1 restate-server [RT0002] configuration loading error: duplicate field `default-num-partitions`

Update: rebuilt from the PR branch and tested again, this no longer happens. Both of these work as expected:

RESTATE_DEFAULT_NUM_PARTITIONS=1 restate-server RESTATE_BOOTSTRAP_NUM_PARTITIONS=1 restate-server

AhmedSoliman · 2025-02-04T11:41:24Z

We agreed that @tillrohrmann will take this over and get ie ready for merge

tillrohrmann · 2025-02-05T08:16:02Z

Taking over this PR now.

Summary: This renames a few configuration and command line arguments while keeping compatibility with old options. This also introduces extra documentation on those configuration keys - `allow-bootstrap` -> `auto-provision` - `bootstrap-num-partitions` -> `default-num-partitions` (aligns with `default-partition-replication`) - `[bifrost.replicated-loglet] default_replication-property` -> `default_log_replication`

pcholakov

Not sure what else is still blocking this but +1 for the renames, and I'm no longer seeing the odd "duplicate key" config error that was happening before.

pcholakov · 2025-02-05T14:57:57Z

crates/types/src/config/bifrost.rs


    /// # Default nodeset size
    ///
    /// Configures the target nodeset size used by the replicated loglet when generating new
    /// nodesets for logs. Setting this to 0 will let the system choose a reasonable value based on
    /// the effective replication_property at the time of logs reconfiguration.
+    ///
+    /// Note that this value only impacts the cluster initial provisioning and will not be respected after
+    /// the cluster has been provisioned.
    // hide the configuration option from serialization if it is the default


Maybe include the same callout here, too?

Suggested change

// hide the configuration option from serialization if it is the default

///

/// To update existing clusters use the `restatectl` utility.

// hide the configuration option from serialization if it is the default

pcholakov · 2025-02-05T15:01:11Z

crates/types/src/config/common.rs

+/// | `auto_provision`        | `allow_bootstrap`           |
+/// | `default_num_partitions` | `bootstrap_num_partitions`  |


pcholakov · 2025-02-05T15:06:16Z

crates/types/src/config/cli_option_overrides.rs

-    pub auto_provision_partitions: Option<bool>,
+    /// Default: 24
+    #[clap(long, global = true, alias = "bootstrap-num-partitions")]
+    pub default_num_partitions: Option<NonZeroU64>,


Update: rebuilt from the PR branch and tested again, this no longer happens. Both of these work as expected:

RESTATE_DEFAULT_NUM_PARTITIONS=1 restate-server RESTATE_BOOTSTRAP_NUM_PARTITIONS=1 restate-server

tillrohrmann · 2025-02-05T16:55:57Z

Merged via 2674511

AhmedSoliman mentioned this pull request Jan 28, 2025

Avoid unnecessary metadata read in cluster controller #2564

Merged

AhmedSoliman force-pushed the pr2566 branch from cd101ac to 4568fbf Compare January 28, 2025 13:07

AhmedSoliman marked this pull request as ready for review January 28, 2025 13:07

AhmedSoliman requested review from tillrohrmann and muhamadazmy January 28, 2025 13:22

AhmedSoliman force-pushed the pr2566 branch 2 times, most recently from 0f47059 to cb70236 Compare January 28, 2025 15:14

tillrohrmann approved these changes Jan 28, 2025

View reviewed changes

tillrohrmann reviewed Jan 28, 2025

View reviewed changes

AhmedSoliman mentioned this pull request Jan 28, 2025

Misc improvements to cluster_controller/logs_controller #2570

Merged

pcholakov reviewed Jan 29, 2025

View reviewed changes

crates/types/src/config/cli_option_overrides.rs Outdated Show resolved Hide resolved

pcholakov reviewed Jan 29, 2025

View reviewed changes

tillrohrmann force-pushed the pr2566 branch from cb70236 to b5d0ce3 Compare February 5, 2025 10:11

Introduce CommonOptionsShadow to support deprecated config options

97ad3f2

tillrohrmann force-pushed the pr2566 branch from b5d0ce3 to 97ad3f2 Compare February 5, 2025 10:26

pcholakov reviewed Feb 5, 2025

View reviewed changes

pcholakov approved these changes Feb 5, 2025

View reviewed changes

tillrohrmann closed this Feb 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1/n][Release] Renames and configuration updates #2566

[1/n][Release] Renames and configuration updates #2566

AhmedSoliman commented Jan 28, 2025 •

edited

Loading

github-actions bot commented Jan 28, 2025 •

edited

Loading

tillrohrmann left a comment

tillrohrmann Jan 28, 2025

tillrohrmann Jan 28, 2025

tillrohrmann Jan 28, 2025

tillrohrmann Jan 28, 2025

AhmedSoliman Jan 29, 2025

tillrohrmann Jan 29, 2025

pcholakov Jan 29, 2025 •

edited

Loading

pcholakov Feb 5, 2025

AhmedSoliman commented Feb 4, 2025

tillrohrmann commented Feb 5, 2025

pcholakov left a comment

pcholakov Feb 5, 2025

tillrohrmann Feb 5, 2025

pcholakov Feb 5, 2025

pcholakov Feb 5, 2025

tillrohrmann commented Feb 5, 2025

		/// \| `auto_provision` \| `allow_bootstrap` \|
		/// \| `default_num_partitions` \| `bootstrap_num_partitions` \|

[1/n][Release] Renames and configuration updates #2566

[1/n][Release] Renames and configuration updates #2566

Conversation

AhmedSoliman commented Jan 28, 2025 • edited Loading

github-actions bot commented Jan 28, 2025 • edited Loading

Test Results

tillrohrmann left a 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

pcholakov Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AhmedSoliman commented Feb 4, 2025

tillrohrmann commented Feb 5, 2025

pcholakov left a 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

tillrohrmann commented Feb 5, 2025

AhmedSoliman commented Jan 28, 2025 •

edited

Loading

github-actions bot commented Jan 28, 2025 •

edited

Loading

pcholakov Jan 29, 2025 •

edited

Loading