Batch up outgoing read-receipts to reduce federation traffic. #4890
Conversation
Rate-limit outgoing read-receipts as per #4730.
richvdh
force-pushed
the
rav/rr_batching
branch
from
936d317 to
1de40fe
Compare
Codecov Report
@@ Coverage Diff @@
## develop #4890 +/- ##
===========================================
+ Coverage 77.94% 77.96% +0.02%
===========================================
Files 326 326
Lines 33958 34008 +50
Branches 5601 5609 +8
===========================================
+ Hits 26467 26514 +47
Misses 5871 5871
- Partials 1620 1623 +3 |
| origin=self._server_name, | ||
| destination=self._destination, | ||
| edu_type="m.receipt", | ||
| content=self._pending_rrs, |
There was a problem hiding this comment.
I'm a bit nervous about not limiting the number of receipts going in here tbh. Can we limit it to less than 100 at least?
There was a problem hiding this comment.
I guess, though given it's a 3-level dict, that's not completely trivial. What are your concerns?
There was a problem hiding this comment.
My concern is that we end up sending 100s of read receipts in a single transaction, causing the remote to fall over. It feels a bit weird to bother limiting EDUs if we end up circumventing that for read receipts.
There was a problem hiding this comment.
If the far end falls over because it has received too many RRs, we ought to spec a limit rather than trusting other servers not to DoS us.
I'd rather hope that we won't end up with that many RRs queued up. But ok, suppose we decide to limit the number of RRs in an EDU. Presumably you'd actually want me to limit the number in a transaction rather than just sending two edus with 100 RRs in each in a single transaction?
It just feels like a bunch of work that I'm not convinced is justified. It's certainly not a nit.
There was a problem hiding this comment.
IRL conversation: it's not entirely clear what would be the right thing to do here. Let's just get it shipped
Rate-limit outgoing read-receipts as per #4730.