Add ability to shard the federation sender #7798
Conversation
erikjohnston
force-pushed
the
erikj/shard_federation_sender
branch
from
85251e9
to
83fed31
Compare
erikjohnston
force-pushed
the
erikj/shard_federation_sender
branch
2 times, most recently
from
5895375
to
3f0613e
Compare
erikjohnston
force-pushed
the
erikj/shard_federation_sender
branch
from
3f0613e
to
04fb6f5
Compare
Attributes in `Config` object should be accessed via the sections, i.e. `config.server.start_pushers` rather than `config.start_pushers`, however a lot of code still uses the latter. If we set `config.start_pushers` then only that only changes the config for the latter style lookup.
| # If multiple federation senders are not defined we always return true. | ||
| if not self.instances or len(self.instances) == 1: | ||
| return True |
There was a problem hiding this comment.
I don't think this can change after creation, so should we do this in FederationConfig() and have a MonolithFederationSendingConfig which always returns True fro should_send_to?
There was a problem hiding this comment.
I thought about this and I think its more lines of code for not a lot. Sending federation requests is sufficiently expensive that I don't think avoiding two boolean checks really makes a difference
| for domain in federation_domain_whitelist: | ||
| self.federation_domain_whitelist[domain] = True |
There was a problem hiding this comment.
Not really related to this PR, but we should probably normalize these domains.
Also, can't this be a set() instead of a dict()?
There was a problem hiding this comment.
I'd rather not try and deal with that here.
| if destination == self.server_name: | ||
| continue |
There was a problem hiding this comment.
Is it worth uplifting the check of whether to not self-send into should_send_to? It seems to be done in many places already!
There was a problem hiding this comment.
Possibly, but I think that'll risk conflating different ideas in a way that is going to lead to confusion down the line
synapse/storage/data_stores/main/schema/delta/58/10federation_pos_instance_name.sql
Show resolved
Hide resolved
|
|
||
| sent_on_1 = False | ||
| sent_on_2 = False | ||
| for i in range(20): |
There was a problem hiding this comment.
This loops seems a bit hammer-ish, can't we choose server names such that we know they'll be shared separately? Or do you not want this test to have any knowledge of the sharding algorithm?
Alternately, it could make sense to use a test-sharding class which does something simpler. Of course the sharding logic would be tested then, but it could be tested with a lower-level unit tests.
There was a problem hiding this comment.
Err, i was planning on using hard coded domains but i forget why I didn't :/ But I'm not terribly concerned with this style if I'm honest.
There was a problem hiding this comment.
I'm not super concerned either, there's I suppose a small chance of a flaky test, but....
* commit 'f299441cc': Add ability to shard the federation sender (#7798)
No description provided.