feat(operator): Add Role::{fixed,estimated}_replica_count#1241
feat(operator): Add Role::{fixed,estimated}_replica_count#1241sbernauer wants to merge 7 commits into
Role::{fixed,estimated}_replica_count#1241Conversation
sbernauer
changed the title
Role::fixed_replica_count and Role::estimated_replica_count helper functions
sbernauer
changed the title
Role::fixed_replica_count and Role::estimated_replica_count helper functionsRole::fixed_replica_count and Role::estimated_replica_count
| /// This is the case when all `replicas` are set to [`Some<u16>`], in which case they are simply | ||
| /// summed. |
There was a problem hiding this comment.
This especially mentions all replicas fields. The function however doesn't assert that this is actually the case in all role groups.
So either this comment needs adjusting, or the function body needs to change.
There was a problem hiding this comment.
As soon as a single rolegroup has None (or 0 for that matter), None is returned. See your other comment in https://github.com/stackabletech/operator-rs/pull/1241/changes#r3512315258
| // The individual replicas are [`u16`]s, so a [`u32`] sum has plenty of space. | ||
| Some(replicas) => Some(u32::from(replicas)), | ||
| }) | ||
| .sum() |
There was a problem hiding this comment.
I'm curious: Does sum flatten the Options produced by the map above?
There was a problem hiding this comment.
sum on an iterator of Options returns None if there is at least one None present in the iterator. If all are Some the sum is returned.
E.g. the replica_counts_with_one_replica_unset test showcases this
Techassi
moved this from Development: Waiting for Review
to Development: In Review
in Stackable Engineering
Techassi
changed the title
Role::fixed_replica_count and Role::estimated_replica_countRole::{fixed,estimated}_replica_count
Co-authored-by: Techassi <git@techassi.dev>
| fn replica_counts_with_all_replicas_set() { | ||
| let role = construct_role_with_replicas([Some(3), Some(2), Some(5)]); | ||
|
|
||
| assert_eq!(role.fixed_replica_count(false), Some(10)); |
There was a problem hiding this comment.
Maybe it's just me and this is non-blocking but these kinds of "true/false" boolean parameters are annoying. What does it mean? I now need to check the function. Can we use an enum ZeroReplicas instead? TreatAsNone vs. Count or something like that?
There was a problem hiding this comment.
I had the same idea but wondered if this is an overkill: d3cc0a2
3 participants
Description
Needed for stackabletech/nifi-operator#953, we can then also use it in https://github.com/stackabletech/trino-operator/blob/3158402b23edde1633b2639eb50c67bb1b41889c/rust/operator-binary/src/crd/mod.rs#L918
Definition of Done Checklist
Author
Reviewer
Acceptance