Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(subscriber): Don't save poll_ops if no-one is receiving them #501

Merged
merged 3 commits into from
Jan 22, 2024
Merged

fix(subscriber): Don't save poll_ops if no-one is receiving them #501

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4063632
fix(subscriber): Don't save poll_ops if no-one is receiving them
grahamking Dec 14, 2023
edbce56
Include feedback
grahamking Dec 16, 2023
2838416
Merge branch 'main' into poll-ops-only-on-watch
hds Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 18 additions & 14 deletions console-subscriber/src/aggregator/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,22 @@
use super::{Command, Event, Shared, Watch};
use crate::{
stats::{self, Unsent},
ToProto, WatchRequest,
};
use console_api as proto;
use proto::resources::resource;
use tokio::sync::{mpsc, Notify};

use std::{
sync::{
atomic::{AtomicBool, Ordering::*},
Arc,
},
time::{Duration, Instant},
};

use console_api as proto;
use proto::resources::resource;
use tokio::sync::{mpsc, Notify};
use tracing_core::{span::Id, Metadata};

use super::{Command, Event, Shared, Watch};
use crate::{
stats::{self, Unsent},
ToProto, WatchRequest,
};

Comment on lines +8 to +19
Copy link
Member

Choose a reason for hiding this comment

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

this is a minor nit, but were these imports reordered by rustfmt or some other editor feature? and if so, can you ensure that you're using the default rustfmt settings? i'm open to merging style changes such as reordering imports, but would prefer to make them in separate PRs from those that make functional changes, and to ensure that if someone else edits this file with the default rustfmt settings, that the change won't be undone. this helps to minimize churn in Git diffs.

thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT yes this is default rustfmt.

I was confused too by this. I have rustfmt 1.7.0-nightly (a577704 2023-11-16). It definitely isn't loading a .rustfmt.toml (or rustfmt.toml). I straced it and it tries a lot of different paths, all of them are "No such file or directory". I ran rustfmt from stable too. Same result.

Originally I was going to leave that part our of the PR, but I figured if it really is default rustfmt, it should be included.

Let me know if you want me to remove it.

mod id_data;
mod shrink;
use self::id_data::{IdData, Include};
Expand Down Expand Up @@ -269,6 +270,9 @@ impl Aggregator {
.drop_closed(&mut self.resource_stats, now, self.retention, has_watchers);
self.async_ops
.drop_closed(&mut self.async_op_stats, now, self.retention, has_watchers);
if !has_watchers {
self.poll_ops.clear();
}
}

/// Add the task subscription to the watchers after sending the first update
Expand Down Expand Up @@ -305,14 +309,10 @@ impl Aggregator {
}

fn resource_update(&mut self, include: Include) -> proto::resources::ResourceUpdate {
let new_poll_ops = match include {
Include::All => self.poll_ops.clone(),
Include::UpdatedOnly => std::mem::take(&mut self.poll_ops),
};
proto::resources::ResourceUpdate {
new_resources: self.resources.as_proto_list(include, &self.base_time),
stats_update: self.resource_stats.as_proto(include, &self.base_time),
new_poll_ops,
new_poll_ops: std::mem::take(&mut self.poll_ops),
dropped_events: self.shared.dropped_resources.swap(0, AcqRel) as u64,
}
}
Expand Down Expand Up @@ -472,6 +472,10 @@ impl Aggregator {
task_id,
is_ready,
} => {
// CLI doesn't show historical poll ops, so don't save them if no-one is watching
if self.watchers.is_empty() {
grahamking marked this conversation as resolved.
Show resolved Hide resolved
return;
}
let poll_op = proto::resources::PollOp {
metadata: Some(metadata.into()),
resource_id: Some(resource_id.into()),
Expand Down