element-hq / element-hq/synapse
Standardise on `current_stream_pos` nomenclature for current stream positions
- Dominant language
- Python
- Stars
- 4.6k
- Forks
- 600
- Avg merge
- 5d 22h
- Merged PRs (30d)
- 51
Description
Spawning from https://github.com/element-hq/synapse/pull/19453#discussion_r3066142328 (cc @MadLittleMods)
We have several ways in the code that we refer to the 'minimum position of all writers to a stream'; essentially the latest position that readers can read up to:
- `stream_token` e.g. `get_to_device_stream_token`
- `current_key` e.g. `self.sources.account_data.get_current_key()`
- `max_stream_id` e.g. `get_max_push_rules_stream_id`
- `current_stream_id` e.g. `get_current_quarantined_media_stream_id`
The `max_stream_id` name is particularly confusable with 'max allocated', which is the 'maximum position of all writers to a stream'.
(This `max_stream_id` name is also what the 'new' streams — `thread_subscriptions`, `sticky_events`, [profile updates](https://github.com/element-hq/synapse/pull/19556/changes#diff-54410b5182b30e6383564c8784e6ffa9c9f6c7bd25647ed1dc2e62c6e7c270d4R331) and media quarantine, have been using. I think this stemmed from me cribbing from `account_data` which uses this name and then writing the ['Cheatsheet for creating a new stream'](https://element-hq.github.io/synapse/v1.151/development/synapse_architecture/streams.html#cheatsheet-for-creating-a-new-stream) with that, which others have referred to)
Would be nice to unify these to a single name (though note: it might make sense to exclude the events stream from the discussion, since we have both a min and max there and therefore it's hard to drop the 'max' name.... although we call those the room_min and room_max even though it's about the events stream :S).
I sought some opinions from the team ([internal](https://matrix.to/#/!SGNQGPGUwtcPBUotTL:matrix.org/$kq59YvgvG0R-toBHEwSwty20PRa6LzGv2VwyGWP9rm0?via=jki.re&via=element.io&via=matrix.org)), which I summarise here:
- @erikjohnston > I'd probably go with something like current_token . A stream_token sounds like it could be any valid token, and max_token sounds like it could be the maximum issued integer ID potentially
- @anoadragon453 > \-0.2 from me on token is that we have a sync token which contains structured data within it. An id just sounds like a number.
- @erikjohnston > I'm not a mad fan of using id as they *aren't* IDs but instead are positions
- @anoadragon453 > pos sounds good to me. We have used stream_pos(ition) in the past, if I recall.
- @reivilibre likes `current_pos` (unifies the above two points)
- @anoadragon453 > I like including `stream`. `current_stream_pos`?
I will go through and update our stream docs and what I can of the codebase to follow this new convention that has settled.
Contributor guide
Assessment
This issue has not been assessed yet.