This repository was archived by the owner on Apr 26, 2024. It is now read-only.
Fix Redis reconnection logic#7482
Merged
erikjohnston merged 4 commits intodevelopfrom May 13, 2020
Merged
Conversation
Member
erikjohnston
force-pushed
the
erikj/fix_positions_startup
branch
from
dbc964d to
6202254
Compare
clokep
reviewed
May 12, 2020
| async def on_REPLICATE(self, conn: AbstractConnection, cmd: ReplicateCommand): | ||
| self.send_positions_to_connection(conn) | ||
|
|
||
| def send_positions_to_connection(self, conn: AbstractConnection): |
Member
There was a problem hiding this comment.
This doesn't seem to use conn at all. Is that on purpose?
Member
Author
There was a problem hiding this comment.
Oh, that's exciting! It should only be sending stuff to the given connection via conn.send_command but instead is sending it to all connections via self.send_command. It doesn't actually matter that we send POSITION spuriously to random connections, but lets fix it
clokep
approved these changes
May 12, 2020
Member
|
some notes for the record: I didn't think this was necessary when I did #7427, since each process will send out a |
phil-flex
pushed a commit
to phil-flex/synapse
that referenced
this pull request
Jun 16, 2020
Proactively send out `POSITION` commands (as if we had just received a `REPLICATE`) when we connect to Redis. This is important as other instances won't notice we've connected to issue a `REPLICATE` command (unlike for direct TCP connections). This is only currently an issue if master process reconnects without restarting (if it restarts then it won't have written anything and so other instances probably won't have missed anything).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This does two things:
1. Ensures we only send aActually, this is spurious.REPLICATEwhen we (re)connect after we successfully subscribe. This ensures we are subscribed to hear responses toREPLICATE(in practice this is unlikely to be an issue as we'll almost certainly finish subscribing by the time other instances have received and responded to theREPLICATE).2. Proactively send out
POSITIONcommands (as if we had just received aREPLICATE) when we connect to Redis. This is important as other instances won't notice we've connected to issue aREPLICATEcommand (unlike for direct TCP connections). This is only currently an issue if master process reconnects without restarting (if it restarts then it won't have written anything and so other instances probably won't have missed anything).