Use vector clocks for room stream tokens.

[erikjohnston]

Currently when using multiple event persisters we (in the worst case) don't tell clients about events until all event persisters have persisted new events after the original event. This is a suboptimal, especially if one of the event persisters goes down.

To handle this, we encode the position of each event persister in the room tokens so that we can send events to clients immediately. To reduce the size of the token we do two things:

1. We create a unique immutable persistent mapping between instance names and a generated small integer ID, which we can encode in the tokens instead of the instance name; and
2. We encode the "persisted upto position" of the room token and then only explicitly include instances that have positions strictly greater than that.

The new tokens look something like: m3478.1~3488.2~3489, where the first number is the min position, and the subsequent - separated pairs are the instance ID to positions map. (We use . and ~ as separators as they're URL safe and not already used by StreamToken).

Unit tests and SyTest support are incoming

[anoadragon453]

words

[richvdh]

I'm somewhat failing to convince myself that this is correct. Why does the presence of a topological component mean that we have to ignore that bound?

[richvdh]

ok, it kinda makes sense having stared at _paginate_room_events_txn for a while, but in terms of this function having a comprehensible interface, I think you need to say it only deals with stream orderings, and to set to_token/from_token = None in _paginate_room_events_txn.

[erikjohnston]

I've changed it so that _filter_results now checks historical tokens as well, that seemed easiest and most consistent.

[richvdh]

I think if we're going to exclude self.instance_map here, we better document what it actually means.

[erikjohnston]

Actually, given its only used in one place I think I might just move it inline, as it makes more sense there why we are excluding instance map part

[erikjohnston]

I then proceeded to use it to fix the other issues, so have kept it

[erikjohnston]

I wonder if we should lift these comparisons into PersistedEventPosition (and give PersistedEventPosition the event's topological ordering), then we could just do lower_token < event_pos < upper_token?

[richvdh]

One problem with overloading < in that way is that, given the different ways we have of comparing token orderings (ref the existence of PersistedEventPosition.persisted_after), < fails to make it clear which mechanism we are using.

So yes, I think it might be nice to move the comparison into PersistedEventPosition, but it might be better to do it with explicit methods rather than <.

(also, note that lower_token < event_pos would call RoomStreamToken.__lt__, so you'd have to put logic in RoomStreamToken too)

[erikjohnston]

(I believe persisted_after has the same semantics)

[richvdh]

generally seems good. a few nits.

[richvdh]

for symmetry, we strictly ought to have a validator for topological which checks that it isn't set after instance_map.

But better, since the token is frozen, just have an __attrs_post_init__ which checks for consistency.

[richvdh]

Suggested change

	type=Dict[str, int], factory=dict, validator=attr.validators.instance_of(dict)
	type=Dict[str, int], factory=dict, validator=attr.validators.deep_mapping(
	key_validator=attr.validators.instance_of(str),
	value_validator=attr.validators.instance_of(int),
	mapping_validator=attr.validators.instance_of(dict),
	),

😇

[richvdh]

Suggested change

	"""Get the stream position that the writer was at at this token.
	"""Get the stream position that the given writer was at at this token.

[richvdh]

One problem with overloading < in that way is that, given the different ways we have of comparing token orderings (ref the existence of PersistedEventPosition.persisted_after), < fails to make it clear which mechanism we are using.

So yes, I think it might be nice to move the comparison into PersistedEventPosition, but it might be better to do it with explicit methods rather than <.

(also, note that lower_token < event_pos would call RoomStreamToken.__lt__, so you'd have to put logic in RoomStreamToken too)

[richvdh]

lgtm