Claim local one-time-keys in bulk #16565
Conversation
Otherwise symbols like `count` start to clash. Also my brain hurts.
DMRobertson
force-pushed
the
dmr/batch_keys_claim
branch
from
62fc3bd
to
619c34d
Compare
I'm gonna iterate over it multiple times.
DMRobertson
force-pushed
the
dmr/batch_keys_claim
branch
3 times, most recently
from
cf5f79f
to
9c03551
Compare
We could probably use executemany? But I don't care.
DMRobertson
force-pushed
the
dmr/batch_keys_claim
branch
from
9c03551
to
f34624b
Compare
DMRobertson
changed the title
|
I think the sytest failure is a flake. Waiting for sytest+workers to finish but I'm gonna assume they pass. |
|
I kicked them off again, not sure if they flaked twice or not though. |
| seen_user_device: Set[Tuple[str, str]] = set() | ||
| for user_id, device_id, _, _, _ in otk_rows: | ||
| if (user_id, device_id) in seen_user_device: | ||
| continue | ||
| seen_user_device.add((user_id, device_id)) | ||
| self._invalidate_cache_and_stream( | ||
| txn, self.count_e2e_one_time_keys, (user_id, device_id) | ||
| ) |
There was a problem hiding this comment.
The old code never used to have this deduplication. If the same (user, device) showed up twice (either with multiple algorithms or claiming multiple keys) we'd send out more than one invalidation for that (user, device). I don't know how much this matters in practice.
There was a problem hiding this comment.
I don't know how much this matters in practice.
I suspect it is just inefficient, but doesn't matter too much?
|
And I think the same question as #16570 -- any idea if this is unit tested? I think it has decent tests though. |
Co-authored-by: Patrick Cloke <[email protected]>
Similar deal. There are tests for handling one key at a time, but none for bulk. I can cobble one together. |
🏓 back you, Patrick. I'm not super thrilled with how the test came out and it feels a little too abstract. WDYT? |
|
(Assuming the thread I left is accurate) |
Co-authored-by: Patrick Cloke <[email protected]>
One of the performance optimsations that Rich spotted in #16554. Another in #16570.
See https://gist.github.com/DMRobertson/243121754aed82eff56fa8ec5181184a for how I convinced myself that the SQL query was legit.
Commitwise reviewable, but sitting in CI over the weekend to see if this works.