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

refactor: remove User last_connect #712

Merged
merged 2 commits into from
Jun 7, 2024

Conversation

taddes
Copy link
Contributor

@taddes taddes commented Jun 5, 2024

Closes SYNC-4248

Note a lot of the existing DynamoDB stuff will be removed in upcoming PR, so for now just removed anything related to this and not the surrounding context.

@taddes taddes self-assigned this Jun 5, 2024
@taddes taddes requested review from pjenvey and jrconlin June 5, 2024 13:58
@@ -128,7 +128,6 @@ router record for a `UAID`.
| router_type | Router Type (See [`autoendpoint::extractors::routers::RouterType`]) |
| node_id | Hostname of the connection node the client is connected to. |
| connected_at | Precise time (in milliseconds) the client connected to the node. |
| last_connect | **global secondary index** - year-month-hour that the client has last connected. |
Copy link
Member

Choose a reason for hiding this comment

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

Since this is already legacy, I wonder if we should keep it?

The page already discusses things like curmonth, which is also a DDB thing back when we did table rotations, so keeping the last_connect is about as equally relevant and historic.

If we just wanted to clean things up, we'd drop all the legacy DDB documentation as well and just keep stuff that reflects the current state.

@@ -45,10 +45,9 @@ 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.

jrconlin
jrconlin previously approved these changes Jun 6, 2024
@taddes taddes force-pushed the refactor/SYNC-4248_remove_user_last_connect branch from 4bda5c7 to 5f4a99c Compare June 7, 2024 19:02
@taddes
Copy link
Contributor Author

taddes commented Jun 7, 2024

Sry @jrconlin , need to re-approve after adding those blubs back.

@taddes taddes requested a review from jrconlin June 7, 2024 19:03
@taddes
Copy link
Contributor Author

taddes commented Jun 7, 2024

Thx @pjenvey

@taddes taddes merged commit 35b698b into master Jun 7, 2024
1 check passed
@taddes taddes deleted the refactor/SYNC-4248_remove_user_last_connect branch June 7, 2024 19:35
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