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

refactor: remove User last_connect #712

Merged
merged 2 commits into from
Jun 7, 2024
Merged

refactor: remove User last_connect #712

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2226886
r remove user last_connect in code and docs references
taddes Jun 5, 2024
5f4a99c
return legacy section docs
taddes Jun 7, 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
3 changes: 0 additions & 3 deletions autoconnect/autoconnect-ws/autoconnect-ws-sm/src/unidentified.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,6 @@ impl UnidentifiedClient {
return Err(SMErrorKind::AlreadyConnected.into());
}
user.connected_at = connected_at;
user.set_last_connect();
if !self.app_state.db.update_user(&mut user).await? {
let _ = self.app_state.metrics.incr("ua.already_connected");
return Err(SMErrorKind::AlreadyConnected.into());
Expand All @@ -150,8 +149,6 @@ impl UnidentifiedClient {
// change from the previous state machine impl)
}

// TODO: NOTE: A new User doesn't get a `set_last_connect()` (matching
// the previous impl)
let user = User {
current_month: self
.app_state
Expand Down
1 change: 0 additions & 1 deletion autopush-common/src/db/bigtable/bigtable_client/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1459,7 +1459,6 @@ mod tests {
router_type: "webpush".to_owned(),
connected_at,
router_data: None,
last_connect: Some(connected_at),
node_id: Some(node_id.clone()),
..Default::default()
};
Expand Down
9 changes: 0 additions & 9 deletions autopush-common/src/db/dynamodb/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ use crate::util::sec_since_epoch;

use async_trait::async_trait;
use cadence::{CountedExt, StatsdClient};
use chrono::Utc;
use rusoto_core::credential::StaticProvider;
use rusoto_core::{HttpClient, Region, RusotoError};
use rusoto_dynamodb::{
Expand Down Expand Up @@ -652,11 +651,3 @@ impl DbClient for DdbClientImpl {
"DynamoDb".to_owned()
}
}

/// Indicate whether this last_connect falls in the current month
pub(crate) fn has_connected_this_month(user: &User) -> bool {
user.last_connect.map_or(false, |v| {
let pat = Utc::now().format("%Y%m").to_string();
v.to_string().starts_with(&pat)
})
}
22 changes: 0 additions & 22 deletions autopush-common/src/db/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,6 @@ use serde::Serializer;
use serde_derive::{Deserialize, Serialize};
use uuid::Uuid;

#[cfg(feature = "dynamodb")]
use crate::db::dynamodb::has_connected_this_month;
use util::generate_last_connect;

#[cfg(feature = "bigtable")]
pub mod bigtable;
pub mod client;
Expand All @@ -33,7 +29,6 @@ pub mod error;
pub mod models;
pub mod reporter;
pub mod routing;
mod util;

// used by integration testing
pub mod mock;
Expand Down Expand Up @@ -183,11 +178,6 @@ pub struct User {
pub router_type: String,
/// Router-specific data
pub router_data: Option<HashMap<String, serde_json::Value>>,
/// Keyed time in a month the user last connected at with limited
/// key range for indexing
// [ed. --sigh. don't use custom timestamps kids.]
#[serde(skip_serializing_if = "Option::is_none")]
pub last_connect: Option<u64>,
/// Last node/port the client was or may be connected to
#[serde(skip_serializing_if = "Option::is_none")]
pub node_id: Option<String>,
Expand Down Expand Up @@ -216,7 +206,6 @@ impl Default for User {
connected_at: ms_since_epoch(),
router_type: "webpush".to_string(),
router_data: None,
last_connect: Some(generate_last_connect()),
node_id: None,
record_version: Some(USER_RECORD_VERSION),
current_month: None,
Expand All @@ -226,17 +215,6 @@ impl Default for User {
}
}

impl User {
#[cfg(feature = "dynamodb")]
pub fn set_last_connect(&mut self) {
self.last_connect = if has_connected_this_month(self) {
None
} else {
Some(generate_last_connect())
}
}
}

/// A stored Notification record. This is a notification that is to be stored
/// until the User Agent reconnects. These are then converted to publishable
/// [crate::db::Notification] records.
Expand Down
15 changes: 0 additions & 15 deletions autopush-common/src/db/util.rs
View file
Open in desktop

This file was deleted.

13 changes: 7 additions & 6 deletions docs/src/table_rotation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,19 +36,20 @@ on the prior month may then be lowered.
## DynamoDB Rotating Message Table Interaction Rules (legacy)

Due to the complexity of having notifications spread across two tables,
several rules are used to avoid losing messages during the month
several rules were used to avoid losing messages during the month
transition.

The logic for connection nodes is more complex, since only the
connection node knows when the client connects, and how many messages it
has read through.

When table rotation is allowed, the router table uses the `curmonth`
field to indicate the last month the client has read notifications
through. This is independent of the last_connect since it is possible
Copy link
Member

Choose a reason for hiding this comment

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

Again I can see us keeping this (and maybe the verbs to past tense) since it refers to table rotation, which we stopped doing even for DDB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You know, I had the same thought when I was taking it out. Given it's clearly labeled legacy, we can let it stay.

When table rotation was allowed, the router table used the `curmonth`
field to indicate the last month the client had read notifications
through. This was independent of the last_connect since it was possible
for a client to connect, fail to read its notifications, then reconnect.
This field is updated for a new month when the client connects **after**
it has ack'd all the notifications out of the last month.
This field was updated for a new month when the client connected **after**
it had ack'd all the notifications out of the last month.


To avoid issues with time synchronization, the node the client is
connected to acts as the source of truth for when the month has flipped
Expand Down