Skip to content

Stop producing values when there are no consumers#6

Merged
shsms merged 5 commits into
frequenz-floss:v0.x.xfrom
shsms:graceful-failures
Jul 18, 2025
Merged

Stop producing values when there are no consumers#6
shsms merged 5 commits into
frequenz-floss:v0.x.xfrom
shsms:graceful-failures

Conversation

@shsms
Copy link
Copy Markdown
Collaborator

@shsms shsms commented Jul 15, 2025

Closes #2

shsms added 4 commits July 14, 2025 16:26
This is more specific, and would make more sense when we add `Ended`
as an additional status.

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
@shsms shsms requested review from Copilot and niklas-timpe July 15, 2025 15:13
@shsms shsms added the cmd:skip-release-notes It is not necessary to update release notes for this PR label Jul 15, 2025
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR stops producing and forwarding samples when there are no active consumers by dropping unused formulas and resamplers, and refines stream status handling in the microgrid client.

  • In logical_meter_actor, track subscriber removal to drop formulas and their dependent resamplers.
  • In microgrid_client_actor, rename Succeeded to Connected, introduce Ended, and adjust send logic to skip when no receivers.
  • Simplify streaming loops to rely on subscriber counts rather than error-driven breaks.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
src/logical_meter/logical_meter_actor.rs Drop unused formulas and associated resamplers
src/client/microgrid_client_actor.rs Update StreamStatus, send logic, and end-of-stream handling
Comments suppressed due to low confidence (2)

src/logical_meter/logical_meter_actor.rs:221

  • [nitpick] Consider renaming formulas_to_drop to formulas_to_remove or keys_to_remove for clearer intent.
        let mut formulas_to_drop = vec![];

src/client/microgrid_client_actor.rs:270

  • Add unit or integration tests to verify that the stream stops and the Ended status is sent when all subscribers drop their receivers.
        if tx.receiver_count() == 0 {

Comment thread src/logical_meter/logical_meter_actor.rs
Comment on lines +221 to +238
let mut formulas_to_drop = vec![];
for (formula_str, formula) in formulas.iter_mut() {
let result = formula.formula.calculate(&comp_data).map_err(|e| {
Error::formula_engine_error(format!("Failed to evaluate formula: {e}"))
})?;
formula
.sender
.send(Sample::new(self.next_ts, result))
.map_err(|_| Error::internal("Failed to send sample for formula".to_string()))?;

if let Err(e) = formula.sender.send(Sample::new(self.next_ts, result)) {
tracing::debug!("No remaining subscribers for formula: {formula_str}. Err: {e}");
formulas_to_drop.push(formula_str.to_string());
}
}

for formula_str in &formulas_to_drop {
if let Some(formula) = formulas.remove(formula_str) {
tracing::debug!("Dropping formula: {}", formula_str);
drop(formula);
}
}
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

[nitpick] You can simplify removal by using formulas.retain(|k, _| !formulas_to_drop.contains(k)) instead of collecting keys and looping twice, which reduces allocations and improves readability.

Copilot uses AI. Check for mistakes.
Comment on lines +240 to +243
let mut components = HashSet::<u64>::new();
for (_, formula) in formulas.iter() {
components.extend(formula.formula.components());
}
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider extracting the component-collection logic into a helper function (e.g., collect_active_components(&formulas)) to improve clarity and make it easier to test in isolation.

Suggested change
let mut components = HashSet::<u64>::new();
for (_, formula) in formulas.iter() {
components.extend(formula.formula.components());
}
let components = collect_active_components(formulas);

Copilot uses AI. Check for mistakes.
Comment on lines +29 to +30
Connected(u64),
Ended(u64),
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

[nitpick] Since Connected and Ended variants are currently handled the same way, consider either consolidating them or adding comments that explain the semantic difference to future readers.

Suggested change
Connected(u64),
Ended(u64),
Inactive(u64),

Copilot uses AI. Check for mistakes.
);
break;
}
if tx.send(data).is_err() {
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

Silently continuing on send errors may hide unexpected conditions; consider logging a debug message when tx.send fails to help diagnose issues in production.

Suggested change
if tx.send(data).is_err() {
if tx.send(data).is_err() {
tracing::debug!(
"Failed to send data for component_id: {:?}. Skipping to next iteration.",
component_id
);

Copilot uses AI. Check for mistakes.
Also switch to a newer version of Ubuntu so that we can get a recent
enough version of the protoc compiler.

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
@shsms shsms force-pushed the graceful-failures branch from 8d3f9bd to b43fede Compare July 15, 2025 15:29
@shsms shsms mentioned this pull request Jul 17, 2025
@shsms shsms added this pull request to the merge queue Jul 18, 2025
Merged via the queue into frequenz-floss:v0.x.x with commit 72c78a0 Jul 18, 2025
3 checks passed
@shsms shsms deleted the graceful-failures branch July 18, 2025 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cmd:skip-release-notes It is not necessary to update release notes for this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support starting and stopping client or logical meter streams on demand

3 participants