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

Handle gappy state with a single snapshot, attempt 3 #363

Merged
merged 12 commits into from
Nov 10, 2023

Conversation

DMRobertson
Copy link
Contributor

@DMRobertson DMRobertson commented Nov 3, 2023

A third iteration of "deal with the gappy poll problem".

I pulled out all the independent bits from #329 to other PRs. What was left fell into two categories: a) the original logic for trying to push out live updates with the gappy poll changes; and b) the logic for nuking connections. This PR consists of b) but not a).

Ancestry: #310 -> #329 -> this.
Closes #294.
Closes #232.

I don't fully remember the details, but may as well pluck this from the
previous PR.
@DMRobertson DMRobertson marked this pull request as ready for review November 3, 2023 18:38
state/accumulator.go Outdated Show resolved Hide resolved
sync2/handler2/handler.go Show resolved Hide resolved
sync3/handler/handler.go Show resolved Hide resolved
h.Dispatcher.OnInvalidateRoom(p.RoomID, joins, invites)

// 3. Destroy involved users' caches.
for _, userID := range involvedUsers {
Copy link
Member

Choose a reason for hiding this comment

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

involvedUsers is potentially very large (think Matrix HQ). It would be better to cut this down to the connected users first, then continue. Otherwise we will spin a lot of cycles/locks on users not even on the upstream HS...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be better to cut this down to the connected users first, then continue.

Does this run the risk of introducing a race?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PTAL at 78b1e59 and 4011e38.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And 9aa8f55 🤦

Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

Please do not merge this yet. I want to land #367 first as a safe deploy before trying a riskier bigger change.

@DMRobertson DMRobertson merged commit cbd3c3c into main Nov 10, 2023
7 checks passed
@DMRobertson DMRobertson deleted the dmr/resnapshot-3 branch November 10, 2023 11:38
DMRobertson pushed a commit that referenced this pull request Nov 10, 2023
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.

Cache invalidation Checkpoint room state
2 participants