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.

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

Implement tracked radios cache #928

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Implement tracked radios cache #928

wants to merge 11 commits into from

Conversation

kurotych
Copy link
Member

@kurotych kurotych commented Jan 8, 2025

The cache updates each time mobile tracker runs

@kurotych kurotych marked this pull request as ready for review January 14, 2025 13:03
Copy link
Contributor

@jeffgrunewald jeffgrunewald left a comment

Choose a reason for hiding this comment

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

unless there's some edge cases around working with the RwLock i believe the calls to clone the arc can be simplified to simply .clone() the the cache struct field value since the arc implements the lightweight reference-counted clone

@@ -241,18 +240,17 @@ impl mobile_config::Gateway for GatewayService {

let (tx, rx) = tokio::sync::mpsc::channel(100);

let radios_cache = Arc::clone(&self.tracked_radios_cache);
Copy link
Contributor

Choose a reason for hiding this comment

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

self.tracked_radios_cache is already an Arc, you should be able to just self.tracked_radios_cache.clone() no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Arc::clone(...) and self.tracked_radios_cache.clone() are actually the same under the hood.
When I use Arc::clone I just want to emphasize (clarity) in the code that it clones the reference (not the value).

Since it is not crucial for me, I've replaced as you ask

pub async fn get_updated_radios(
pool: &Pool<Postgres>,
min_updated_at: DateTime<Utc>,
) -> anyhow::Result<HashMap<PublicKeyBinary, DateTime<Utc>>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
) -> anyhow::Result<HashMap<PublicKeyBinary, DateTime<Utc>>> {
) -> anyhow::Result<TrackedRadiosMap> {


pub type TrackedRadiosMap = HashMap<PublicKeyBinary, DateTime<Utc>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if this would make sense as it's own type

#[derive(Clone)]
pub struct TrackedRadiosMap(Arc<RwLock<HashMap<PublicKeyBinary, DateTime<Utc>>>);

We would get Clone for free.
And we could hang methods like get_updated_radios off of it, and it would self update.
And other access methods could manage the read locking instead of the user.

Just thinking out loud.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants