Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix sending out of order POSITION over replication #16639

Merged
merged 5 commits into from
Nov 16, 2023

Conversation

erikjohnston
Copy link
Member

If a worker reconnects to Redis we send out the current positions of all our streams. However, if we're also trying to send out a backlog of RDATA at the same time then we can end up sending a POSITION with the current token before we've sent all the RDATA before the current token.

This doesn't cause actual bugs as the receiving servers see the POSITION, fetch the relevant rows from the DB, and then ignore the old RDATA as they come in. However, this is inefficient so it'd be better if we didn't send out-of-order positions

@erikjohnston erikjohnston marked this pull request as ready for review November 15, 2023 16:10
@erikjohnston erikjohnston requested a review from a team as a code owner November 15, 2023 16:10
synapse/replication/tcp/handler.py Outdated Show resolved Hide resolved
synapse/replication/tcp/handler.py Show resolved Hide resolved
Comment on lines 162 to 164
# We need to send out POSITIONs for all streams, usually
# because a worker has reconnected.
self.command_handler.will_should_announce_positions()
Copy link
Member

Choose a reason for hiding this comment

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

will_should_announce_positions sets _should_announce_positions to false, so we're essentially saying that we've now announced positions, don't do it again next time around the loop?

Is it possible for another REPLICATE to come in after this? I guess that's fine, we'll just send them again next loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, pretty much.

so we're essentially saying that we've now announced positions, don't do it again next time around the loop?

We say we are about to send positions (to handle the race where something asks us to send out positions after we've just sent them out but before we clear the flag)

@erikjohnston erikjohnston enabled auto-merge (squash) November 16, 2023 13:03
@erikjohnston erikjohnston merged commit fef08cb into develop Nov 16, 2023
38 checks passed
@erikjohnston erikjohnston deleted the erikj/better_replication branch November 16, 2023 13:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants