This repository was archived by the owner on Apr 26, 2024. It is now read-only.
Reduce duplication in event ordering lookup methods#8453
Closed
richvdh wants to merge 1 commit into
Closed
Conversation
richvdh
changed the title
richvdh
force-pushed
the
rav/ordering_method_duplication
branch
from
1be8225 to
798373a
Compare
`get_topological_token_for_event` and `get_event_ordering` were doing almost the same thing: the difference was only in the format of the result (`tuple` vs `RoomStreamOrdering`). This commit combines the two. `get_event_ordering` had a cache which may have been tuned so we stick with that name, but update it to return a `RoomStreamOrdering` for better type safety. We also put it in `StreamWorkerStore` rather than `EventsWorkerStore` since that's where all the other "ordering" methods are. So then we have to update all the places `get_topological_token_for_event` was called, to call `get_event_ordering` instead, and we have to update all the places `get_event_ordering` was called (just `is_event_after`) to handle `RoomStreamToken`s intstead of tuples. `is_event_after` was only called in once place and we may as well inline it.
richvdh
force-pushed
the
rav/ordering_method_duplication
branch
from
798373a to
9a3b3da
Compare
clokep
approved these changes
Oct 5, 2020
| token2 = await self.store.get_event_ordering( | ||
| existing_read_marker["event_id"] | ||
| ) | ||
| should_update = token1 > token2 |
Member
There was a problem hiding this comment.
attr objects by default allow comparisons I'm assuming? (I don't see anything special in RoomStreamToken to support this.)
Member
There was a problem hiding this comment.
Argh, #8439 removes this as RoomStreamTokens should not be comparable (as they'll be vector clocks in some cases).
Member
Author
|
I'm aborting this for now, since @erikjohnston is rewriting the world and it's not on the critical path for my work. |
callahad
added
the
Z-Time-Tracked
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
get_topological_token_for_eventandget_event_orderingwere doing almost the same thing: the difference was only in the format of the result (tuplevsRoomStreamOrdering).This commit combines the two.
get_event_orderinghad a cache which may have been tuned so we stick with that name, but update it to return aRoomStreamOrderingfor better type safety. We also put it inStreamWorkerStorerather thanEventsWorkerStoresince that's where all the other "ordering" methods are.So then we have to update all the places
get_topological_token_for_eventwas called, to callget_event_orderinginstead, and we have to update all the placesget_event_orderingwas called (justis_event_after) to handleRoomStreamTokens intstead of tuples.is_event_afterwas only called in once place and we may as well inline it.Based on #8452.