Reduce amount of caches POSITIONS we send #16561
Conversation
| @@ -489,6 +497,8 @@ def current_token(self, instance_name: str) -> Token: | |||
| return self.store.get_cache_stream_token_for_writer(instance_name) | |||
|
|
|||
| def minimal_local_current_token(self) -> Token: | |||
| if self.store._cache_id_gen: | |||
| return self.store._cache_id_gen.get_minimal_local_current_token() | |||
There was a problem hiding this comment.
_cache_id_gen is None on SQLite
There was a problem hiding this comment.
Why is the cache stream special?
There was a problem hiding this comment.
I think at the time we didn't have a good way of doing it on SQLite, or something?
There was a problem hiding this comment.
Sorry, I think I mean two questions:
- Why are we handling the caches stream separately to the others? Presumably b/c it is especially chatty and we want to ratelimit the updates sent out?
- What's the difference between
self.store._cache_id_gen.get_minimal_local_current_token()andself.current_token(self.local_instance_name)?
There was a problem hiding this comment.
Why are we handling the caches stream separately to the others? Presumably b/c it is especially chatty and we want to ratelimit the updates sent out?
This seems to be the case, by my reading of https://github.com/matrix-org/synapse/pull/16557/files#diff-844ba8f7be8c32eb75cc8092e1c48528797f6a8e1eeada942724ba6f73923a9cR214-R218
There was a problem hiding this comment.
Ahh, I'd missed that this was inside the CachesStream class, I thought this was base logic for all streams.
I guess the point is that if there's no _cache_id_gen then there are no other workers to worry about and so the distinction between minimum and current tokens is moot?
There was a problem hiding this comment.
Are there any other streams whose minimal_local_current_token impl we should sanity check?
There was a problem hiding this comment.
I guess the point is that if there's no
_cache_id_genthen there are no other workers to worry about and so the distinction between minimum and current tokens is moot?
Yup
Are there any other streams whose
minimal_local_current_tokenimpl we should sanity check?
I don't think so. Most of the others can just rely directly on the ID gens (and there's a helper class to do that)
There was a problem hiding this comment.
I mean more that e.g. the function bodies here don't call something with the word "minimal" in:
synapse/synapse/replication/tcp/streams/federation.py
Lines 74 to 75 in 8f35f81
synapse/synapse/replication/tcp/streams/_base.py
Lines 389 to 390 in 8f35f81
synapse/synapse/replication/tcp/streams/_base.py
Lines 343 to 344 in 8f35f81
There was a problem hiding this comment.
Right, so yeah you're right that they're suboptimal implementations, but they are also valid implementations. We mostly care about the difference for caches as a) its high traffic, and b) we have an extra check for it that wants minimal_local_current_token to return the actual minimum
Follow on from / actually correctly does #16557