Send reminders for non-mutating channels #77
Conversation
arik-so
force-pushed
the
static-channel-reminders
branch
2 times, most recently
from
b9fff82
to
206b767
Compare
src/lookup.rs
Outdated
| let snapshot_scope = current_timestamp.saturating_sub(last_sync_timestamp as u64); | ||
| log_debug!(logger, "Current day index: {}", current_day); | ||
| log_debug!(logger, "Snapshot scope: {}s", snapshot_scope); | ||
| let include_reminders = ((current_day % 5) == 0 || snapshot_scope > (40 * 3600)); |
There was a problem hiding this comment.
current_day % 5 means any generated snapshot created on the 5th day, not only at midnight.
src/lookup.rs
Outdated
| @@ -87,6 +92,16 @@ pub(super) async fn fetch_channel_announcements<L: Deref>(delta_set: &mut DeltaS | |||
| log_info!(logger, "Last sync timestamp: {}", last_sync_timestamp); | |||
| let last_sync_timestamp_float = last_sync_timestamp as f64; | |||
|
|
|||
| let current_time = SystemTime::now(); | |||
There was a problem hiding this comment.
Please use the snapshot generation time, not the system time.
src/serialization.rs
Outdated
| } else if mutated_properties.len() > 0 || mutated_properties.flags { | ||
| } else if mutated_property_count > 0 || mutated_properties.flags { | ||
| // we don't count flags as mutated properties | ||
| serialization_set.updates.push( | ||
| UpdateSerialization::Incremental(latest_update, mutated_properties)); | ||
| serialization_set.updates.push(UpdateSerialization::Incremental(latest_update, mutated_properties)); |
There was a problem hiding this comment.
Can we skip the unnecessary diff?
There was a problem hiding this comment.
sure, I added that for debugging. Can undo.
There was a problem hiding this comment.
hmm, it's actually kinda convenient for debugging though. Is this critical?
There was a problem hiding this comment.
Why is it convenient to have additional diff for debugging? You can print mutated_properties.len() without needing a local variable for it...
src/lookup.rs
Outdated
| let is_reminder_day = (current_day % 5) == 0; | ||
|
|
||
| let snapshot_scope = current_timestamp.saturating_sub(last_sync_timestamp as u64); | ||
| let is_reminder_scope = snapshot_scope > (40 * 3600); |
There was a problem hiding this comment.
I can set it to 48 hours sharp, that should probably do the trick.
There was a problem hiding this comment.
Do we need it to be that short? Can we do 5 days to match the above?
There was a problem hiding this comment.
We can, but really the trick is to catch it depending on the lowest interval people are gonna set, because it's gonna be a power of two.
With 3 hours as the starting interval, it's gonna be 6, 12, 24, 48, 96, 192.
If users start with 1, it'll be 4, 8, 16, 32, 64, 128. For their case, 128 will be right above 120, which is great, but with 3 hours, the next one is 192, which is more than 6 days, i.e. our reminder interval.
There was a problem hiding this comment.
Mmm, okay, I still feel like it'd be nice to set higher but its fine.
There was a problem hiding this comment.
Right, so how about 50 perhaps? That would start the capture for the 96-hour and 64-hour-intervals.
Previously, whenever a channel hasn't received updates in 6+ days, we would automatically send incremental reminders that only contain the update flags. However, if a channel has been received updates within that six-day-timeframe, but all those updates were identical, it would result in no updates being added to the snapshot because the mutation set ended up empty and that data would get purged from the serialization. In order to avoid the reminder logic being duped by channels simply being consistent, we now look up the beginning of the latest continuous stretch of non-mutating channel updates. If a channel's details have not been altered in more than six days, we now send reminders no matter the frequency with which channel updates have been received since.
arik-so
force-pushed
the
static-channel-reminders
branch
from
57cd704
to
c5fb1c2
Compare
Stop filtering channels from the delta set due to non-mutating channel updates absent which reminders would be sent.