From 2a0fc6cec6b94304ed46597363ba3d9e5fb0c686 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Wed, 8 Jul 2026 16:48:22 +0200 Subject: [PATCH 1/3] refactor!: Make `Role::{fixed,estimated}_replica_count` standalone --- crates/stackable-operator/CHANGELOG.md | 7 + crates/stackable-operator/src/role_utils.rs | 172 ++++++++------------ 2 files changed, 78 insertions(+), 101 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index a7f680ec3..f6e6b2ec2 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -4,6 +4,13 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Changed + +- BREAKING: Make `Role::fixed_replica_count` and `Role::estimated_replica_count` functions standalone, + so consumers don't need access to the `Role` struct ([#XXXX]). + +[#XXXX]: https://github.com/stackabletech/operator-rs/pull/XXXX + ## [0.113.3] - 2026-07-07 ### Fixed diff --git a/crates/stackable-operator/src/role_utils.rs b/crates/stackable-operator/src/role_utils.rs index b0b1dcc8d..43804b2c9 100644 --- a/crates/stackable-operator/src/role_utils.rs +++ b/crates/stackable-operator/src/role_utils.rs @@ -403,62 +403,6 @@ where } } -impl - Role -where - RoleConfig: Default + JsonSchema + Serialize, - CommonConfig: Default + JsonSchema + Serialize, - ConfigOverrides: Default + JsonSchema + Serialize, -{ - /// Returns [`Some`] in case the number of replicas is hard-coded to a certain value. - /// - /// This is the case when all `replicas` are set to [`Some`], in which case they are simply - /// summed. - /// - /// The argument `zero_replicas_counting` is a safety mechanism, which allows the caller to - /// decide if an explicit replica count of `0` should be treated as [`None`]. It also means that - /// [`None`] is returned in case no roleGroups are configured at all. - pub fn fixed_replica_count(&self, zero_replicas_counting: ZeroReplicasCounting) -> Option { - // An empty role has no fixed replica count when zeros are treated as None. - if zero_replicas_counting == ZeroReplicasCounting::TreatAsNone - && self.role_groups.is_empty() - { - return None; - } - - self.role_groups - .values() - .map(|rg| match rg.replicas { - None => None, - Some(0) if zero_replicas_counting == ZeroReplicasCounting::TreatAsNone => None, - // The individual replicas are [`u16`]s, so a [`u32`] sum has plenty of space. - Some(replicas) => Some(u32::from(replicas)), - }) - .sum() - } - - /// Returns the estimated total number of replicas across all role groups. - /// - /// Unlike [`Self::fixed_replica_count`], this always returns a value: a role group with an unset - /// (i.e. [`None`]) replica count is assumed to run a single replica. Use this when a best-effort - /// estimate is needed even though the exact number of replicas is not hard-coded. - pub fn estimated_replica_count(&self) -> u32 { - self.role_groups - .values() - .map(|rg| u32::from(rg.replicas.unwrap_or(1))) - .sum() - } -} - -/// How explicit zero (`0`) replicas on a role group should be counted -#[derive(Copy, Clone, Debug, PartialEq, Eq)] -pub enum ZeroReplicasCounting { - /// Treat them as what they are: `Some(0)`. - TreatAsZero, - /// Treat them as if the user configured [`None`]. - TreatAsNone, -} - impl Role where @@ -594,6 +538,56 @@ impl Display for RoleGroupRef { } } +/// Returns [`Some`] in case the number of replicas is hard-coded to a certain value. +/// +/// This is the case when all `replicas` are set to [`Some`], in which case they are simply +/// summed. +/// +/// The argument `zero_replicas_counting` is a safety mechanism, which allows the caller to +/// decide if an explicit replica count of `0` should be treated as [`None`]. It also means that +/// [`None`] is returned in case no roleGroups are configured at all. +pub fn fixed_replica_count>>( + replicas: I, + zero_replicas_counting: ZeroReplicasCounting, +) -> Option { + let mut replicas = replicas.into_iter().peekable(); + + // An empty role has no fixed replica count when zeros are treated as None. + if zero_replicas_counting == ZeroReplicasCounting::TreatAsNone && replicas.peek().is_none() { + return None; + } + + replicas + .map(|replicas| match replicas { + None => None, + Some(0) if zero_replicas_counting == ZeroReplicasCounting::TreatAsNone => None, + // The individual replicas are [`u16`]s, so a [`u32`] sum has plenty of space. + Some(replicas) => Some(u32::from(replicas)), + }) + .sum() +} + +/// Returns the estimated total number of replicas across all role groups. +/// +/// Unlike [`Self::fixed_replica_count`], this always returns a value: a role group with an unset +/// (i.e. [`None`]) replica count is assumed to run a single replica. Use this when a best-effort +/// estimate is needed even though the exact number of replicas is not hard-coded. +pub fn estimated_replica_count>>(replicas: I) -> u32 { + replicas + .into_iter() + .map(|replicas| u32::from(replicas.unwrap_or(1))) + .sum() +} + +/// How explicit zero (`0`) replicas on a role group should be counted +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum ZeroReplicasCounting { + /// Treat them as what they are: `Some(0)`. + TreatAsZero, + /// Treat them as if the user configured [`None`]. + TreatAsNone, +} + #[cfg(test)] mod tests { use std::collections::HashSet; @@ -713,101 +707,77 @@ mod tests { #[test] fn replica_counts_with_all_replicas_set() { - let role = construct_role_with_replicas([Some(3), Some(2), Some(5)]); + let replicas = [Some(3), Some(2), Some(5)]; assert_eq!( - role.fixed_replica_count(ZeroReplicasCounting::TreatAsZero), + fixed_replica_count(replicas, ZeroReplicasCounting::TreatAsZero), Some(10) ); assert_eq!( - role.fixed_replica_count(ZeroReplicasCounting::TreatAsNone), + fixed_replica_count(replicas, ZeroReplicasCounting::TreatAsNone), Some(10) ); - assert_eq!(role.estimated_replica_count(), 10); + assert_eq!(estimated_replica_count(replicas), 10); } #[test] fn replica_counts_with_one_replica_unset() { - let role = construct_role_with_replicas([Some(3), None, Some(2)]); + let replicas = [Some(3), None, Some(2)]; assert_eq!( - role.fixed_replica_count(ZeroReplicasCounting::TreatAsZero), + fixed_replica_count(replicas, ZeroReplicasCounting::TreatAsZero), None ); assert_eq!( - role.fixed_replica_count(ZeroReplicasCounting::TreatAsNone), + fixed_replica_count(replicas, ZeroReplicasCounting::TreatAsNone), None ); - assert_eq!(role.estimated_replica_count(), 6); + assert_eq!(estimated_replica_count(replicas), 6); } #[test] fn replica_counts_with_a_zero_replica() { - let role = construct_role_with_replicas([Some(3), Some(0)]); + let replicas = [Some(3), Some(0)]; assert_eq!( - role.fixed_replica_count(ZeroReplicasCounting::TreatAsZero), + fixed_replica_count(replicas, ZeroReplicasCounting::TreatAsZero), Some(3) ); // With treat_zero_as_none the zero turns the whole count into None. assert_eq!( - role.fixed_replica_count(ZeroReplicasCounting::TreatAsNone), + fixed_replica_count(replicas, ZeroReplicasCounting::TreatAsNone), None ); - assert_eq!(role.estimated_replica_count(), 3); + assert_eq!(estimated_replica_count(replicas), 3); } #[test] - fn replica_counts_with_a_single_zero_role_group() { - let role = construct_role_with_replicas([Some(0)]); + fn replica_counts_with_a_single_zero_role_groups_group() { + let replicas = [Some(0)]; assert_eq!( - role.fixed_replica_count(ZeroReplicasCounting::TreatAsZero), + fixed_replica_count(replicas, ZeroReplicasCounting::TreatAsZero), Some(0) ); assert_eq!( - role.fixed_replica_count(ZeroReplicasCounting::TreatAsNone), + fixed_replica_count(replicas, ZeroReplicasCounting::TreatAsNone), None ); - assert_eq!(role.estimated_replica_count(), 0); + assert_eq!(estimated_replica_count(replicas), 0); } #[test] fn replica_counts_without_role_groups() { - let role = construct_role_with_replicas(vec![]); + let replicas = []; assert_eq!( - role.fixed_replica_count(ZeroReplicasCounting::TreatAsZero), + fixed_replica_count(replicas, ZeroReplicasCounting::TreatAsZero), Some(0) ); assert_eq!( - role.fixed_replica_count(ZeroReplicasCounting::TreatAsNone), + fixed_replica_count(replicas, ZeroReplicasCounting::TreatAsNone), None ); - assert_eq!(role.estimated_replica_count(), 0); - } - - /// Builds a [`Role`] with one role group per passed `replicas` entry, so tests only need to - /// care about the replica counts that [`Role::fixed_replica_count`] operates on. - fn construct_role_with_replicas( - replicas: impl IntoIterator>, - ) -> Role<(), EmptyConfigOverrides, GenericRoleConfig, GenericCommonConfig> { - Role { - config: CommonConfiguration::default(), - role_config: GenericRoleConfig::default(), - role_groups: replicas - .into_iter() - .enumerate() - .map(|(index, replicas)| { - ( - format!("role-group-{index}"), - RoleGroup { - config: CommonConfiguration::default(), - replicas, - }, - ) - }) - .collect(), - } + assert_eq!(estimated_replica_count(replicas), 0); } } From 9c9c51a72938a0fd391e92738e3f0ae11ed1efdb Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Wed, 8 Jul 2026 16:49:36 +0200 Subject: [PATCH 2/3] changelog --- crates/stackable-operator/CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index f6e6b2ec2..468d21e5c 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -7,9 +7,9 @@ All notable changes to this project will be documented in this file. ### Changed - BREAKING: Make `Role::fixed_replica_count` and `Role::estimated_replica_count` functions standalone, - so consumers don't need access to the `Role` struct ([#XXXX]). + so consumers don't need access to the `Role` struct ([#1247]). -[#XXXX]: https://github.com/stackabletech/operator-rs/pull/XXXX +[#1247]: https://github.com/stackabletech/operator-rs/pull/1247 ## [0.113.3] - 2026-07-07 From c24c510d2520f1eb61fd5d825a89fc720b79f508 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 9 Jul 2026 09:22:34 +0200 Subject: [PATCH 3/3] Document why we use IntoIterator --- crates/stackable-operator/src/role_utils.rs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/crates/stackable-operator/src/role_utils.rs b/crates/stackable-operator/src/role_utils.rs index 43804b2c9..5266c5b73 100644 --- a/crates/stackable-operator/src/role_utils.rs +++ b/crates/stackable-operator/src/role_utils.rs @@ -543,9 +543,12 @@ impl Display for RoleGroupRef { /// This is the case when all `replicas` are set to [`Some`], in which case they are simply /// summed. /// -/// The argument `zero_replicas_counting` is a safety mechanism, which allows the caller to -/// decide if an explicit replica count of `0` should be treated as [`None`]. It also means that -/// [`None`] is returned in case no roleGroups are configured at all. +/// The argument `zero_replicas_counting` is a safety mechanism, which allows the caller to decide +/// if an explicit replica count of `0` should be treated as [`None`]. It also means that [`None`] +/// is returned in case no roleGroups are configured at all. +// +// Note: We are using a [`IntoIterator`] combined with `.peekable()` over [`ExactSizeIterator`] to +// have minimal bound requirements on the caller. pub fn fixed_replica_count>>( replicas: I, zero_replicas_counting: ZeroReplicasCounting, @@ -569,9 +572,12 @@ pub fn fixed_replica_count>>( /// Returns the estimated total number of replicas across all role groups. /// -/// Unlike [`Self::fixed_replica_count`], this always returns a value: a role group with an unset -/// (i.e. [`None`]) replica count is assumed to run a single replica. Use this when a best-effort -/// estimate is needed even though the exact number of replicas is not hard-coded. +/// Unlike [`fixed_replica_count`], this always returns a value: a role group with an unset (i.e. +/// [`None`]) replica count is assumed to run a single replica. Use this when a best-effort estimate +/// is needed even though the exact number of replicas is not hard-coded. +// +// Note: We are using a [`IntoIterator`] combined with `.peekable()` over [`ExactSizeIterator`] to +// have minimal bound requirements on the caller. pub fn estimated_replica_count>>(replicas: I) -> u32 { replicas .into_iter()