chore: Add capability to crash a worker if triggered from main process#274
chore: Add capability to crash a worker if triggered from main process#274jason-famedly wants to merge 3 commits into
Conversation
… then adding the in the sending and receiving processing
jason-famedly
force-pushed
the
jason/desict-redis
branch
from
7e8b974 to
3e07b87
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #274 +/- ##
==========================================
- Coverage 80.43% 80.42% -0.02%
==========================================
Files 502 502
Lines 72214 72252 +38
Branches 10888 10892 +4
==========================================
+ Hits 58088 58109 +21
- Misses 10861 10875 +14
- Partials 3265 3268 +3
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
Only test that failed on initial run was |
| ) | ||
| self.synapse_handler.new_connection(self) | ||
| await self._async_send_command(ReplicateCommand()) | ||
| self.send_command(ReplicateCommand()) |
There was a problem hiding this comment.
why did you remove the await here?
There was a problem hiding this comment.
Look inside send_command(), it does the await as a background task, which feels kinda dumb. Just send the command. No real strong opinions otherwise, just seemed silly
| self.max_event_delay_ms = None | ||
|
|
||
| force_crash_workers_after_main_restart = config.get( | ||
| "force_crash_workers_after_main_restart", False |
There was a problem hiding this comment.
we should probably document this config option somwhere
There was a problem hiding this comment.
yes, good call. Forgot that
| ServerCommand( | ||
| self.server_name, | ||
| self.hs.get_instance_name(), | ||
| self.hs.get_clock().time_msec(), |
There was a problem hiding this comment.
won't this value be the time of the redis connection (re)start? This would mean that a redis restart would crash the worker.
There was a problem hiding this comment.
Hmm. Yes probably, good catch again
2 participants
SYN-53
It has been found that sometimes a worker is missed when restarting containers. This adds a new top-level setting
force_crash_workers_after_main_restart(bool) to enable sending a SIGTERM signal to restart worker when triggered by redis.An existing(although currently unused) redis "command" has been reworked for this purpose to send not only the server's instance name but also the name of the worker sending the command and when that process started. It should be received by any workers currently connected. Any worker receiving this command shall check their own start up time to compare against this received value and, if this should not be the main process, raise a SIGTERM. In theory, may not even really need to include the timestamp itself, as this command alone should only be broadcast on the start of the process.
This command needs to be sent before the redis replication process starts or it may interfere with the SUBSCRIBE and REPLICATE commands that follow since we are borrowing that connection
Note to reviewer: I included the changes into the old deprecated TCP replication system as well. It is completely unused and disconnected, but is still tested by old tests for some unknown reason. It can be safely stripped out when this is removed