Remove old device_connections in worker #1807
Conversation
| days_ago = DateTime.shift(DateTime.utc_now(), day: -interval) | ||
|
|
||
| DeviceConnection | ||
| |> where([d], d.last_seen_at < ^days_ago) |
There was a problem hiding this comment.
@joshk Since there are no timestamps on device_connections is last_seen_at the right column to query against?
There was a problem hiding this comment.
you can actually do this by id, as UUID v7s have the timestamp at the front, but I think a few other things pop out at me first.
We shouldn't delete connections which are still active (where state == connected).
And it would also be good to keep at least one device connection row per device. This current query could remove all device connections for a device if it hasn't reconnected recently.
There was a problem hiding this comment.
Do you think we need limit and batching protection? Only delete X amount at a time and if there is more remaining, delete the next batch (or reschedule job)
There was a problem hiding this comment.
I think if this is run hourly then we will be fine, its the first one which we might want to run manually with some batching.
There was a problem hiding this comment.
You could argue that batching should be the default, in case truncation is enabled for an installation that has a large dataset already, but I don't think anyone is using it yet, or at least to the extremes as SmartRent.
There was a problem hiding this comment.
Batching by default is right here, I'll implement.
| @@ -40,9 +40,6 @@ defmodule Mix.Tasks.NervesHub.Gen.Devices do | |||
| org_id: org.id, | |||
| product_id: product.id, | |||
| identifier: "generated-#{i}", | |||
| connection_status: :connected, | |||
| connection_established_at: DateTime.now!("Etc/UTC"), | |||
| connection_last_seen_at: DateTime.now!("Etc/UTC"), | |||
| connection_metadata: %{ | |||
There was a problem hiding this comment.
since we have metadata on DeviceConnection, I wonder if we remove this field too? (we just need to make sure we save the geo data to the DeviceConnection
nshoes
force-pushed
the
remove-old-device-connections-worker
branch
from
87be70f to
ab30c3d
Compare
| |> join(:inner, [dc], d in Device, on: dc.device_id == d.id) | ||
| |> where([dc, _d], dc.last_seen_at < ^days_ago) | ||
| |> where([dc, _d], dc.status != :connected) | ||
| |> where([dc, d], dc.id != d.latest_connection_id) |
| @@ -468,7 +470,7 @@ defmodule NervesHubWeb.Live.Devices.Show do | |||
| end) | |||
| end | |||
|
|
|||
| defp device_connection(%{device_connections: [connection]}), do: connection | |||
| defp device_connection(%{latest_connection: latest_connection}), do: latest_connection | |||
There was a problem hiding this comment.
this is always going to return something, so I think we can remove this line and the line below
There was a problem hiding this comment.
How will it always return something? device.latest_connection_id will be nil until the device connects to the socket, so there is an in-between period. Should we backfill? Even then, a device could still not have a device_connection, so maybe that's a bad idea.
Defaults to 14 days if env var
DEVICE_CONNECTION_MAX_AGE_DAYSisn't set.