Attempting to add Redis Sentinel support#1
Conversation
| enabled: true | ||
| sentinel: true | ||
| service_name: service_name | ||
| servers: |
There was a problem hiding this comment.
It might be more "correct" to call these sentinels (as you have it in config_documentation.md)
|
|
||
| class SynapseSentinelConnectionFactory(txredisapi.SentinelConnectionFactory): | ||
|
|
||
| maxDelay = 5 |
There was a problem hiding this comment.
Maybe you can store this in a constant or env. var or in homeserver.yaml?
Also, it might be beneficial to add a comment describing the unit of measure (assume it's seconds)
yasinishyn
left a comment
yasinishyn
left a comment
There was a problem hiding this comment.
Minor style issues (subjective).
Overall looks great!
| sentinel: true | ||
| service_name: service_name | ||
| sentinels: | ||
| - host: redis1 |
There was a problem hiding this comment.
Can be minor, but "redis1" suggests that it is Redis, not Sentinel.
| from synapse.server import HomeServer | ||
| from typing import Optional, cast, Type, TypeVar, Generic | ||
| import attr | ||
| import txredisapi |
There was a problem hiding this comment.
Might be cleaner if you group all 'synapse.*' imports together.
|
|
||
| def publish(self, channel, message): ... | ||
|
|
||
|
|
There was a problem hiding this comment.
Should we add comments describing what T and V are for?
| reconnect=True) | ||
|
|
||
|
|
||
| def get_replication_factory(hs: "HomeServer", connection: IRedisConnection, channel_names: List[str]) -> txredisapi.RedisFactory: |
There was a problem hiding this comment.
The multi line method params is used multiple times. Maybe we should apply the same approach to methods which have "long" definition?
| if isinstance(connection, SentinelRedisConnection): | ||
| factory = RedisSentinelReplicationFactory( | ||
| hs, connection.sentinel, connection.service_name, True, connection, channel_names) | ||
| txredisapi.Sentinel._connect_factory_and_return_handler(factory, 1) |
There was a problem hiding this comment.
"1" is a "magic number" should we save it in a constant?
| hs.config.redis.redis_host, | ||
| hs.config.redis.redis_port, | ||
| factory, | ||
| timeout=30, |
| self.sentinel_enabled = redis_config.get("sentinel", False) | ||
| self.service_name = redis_config.get("service_name") | ||
| self.sentinels = [] | ||
| for sentinel in redis_config.get("sentinels", []): |
There was a problem hiding this comment.
Because sentinels is array, I have a feeling that this and bellow line could be done as a single-liner.
roeltm
force-pushed
the
add-redis-sentinel-support
branch
2 times, most recently
from
782fbfd to
7fc2d9c
Compare
roeltm
force-pushed
the
add-redis-sentinel-support
branch
from
d5ffd0c to
311b4b5
Compare
2 participants
No description provided.