element-hq / element-hq/synapse

Standardise on `current_stream_pos` nomenclature for current stream positions

Open
#19,685 0 comments 0 reactions 1 assignee Claimed by @reivilibre View on GitHub
T-Task
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.