Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

state-res: Use closure to fetch unknown events during state-res #654

Merged
merged 1 commit into from
Jul 1, 2021

Conversation

DevinR528
Copy link
Member

Still have to test it out and then change the docs but I think this seems nice.

@DevinR528 DevinR528 force-pushed the state-closure branch 3 times, most recently from 17e63c4 to 95208b9 Compare June 29, 2021 23:48
@ShadowJonathan
Copy link
Member

Does this fire upon every missing event? Wouldn't it be better to allow for more "batch"-like query-esc functions? I can imagine the round trip time ping-ponging the underlying impl (which would likely be a server) querying the event from another room and then going back to the state-res to be quite slow.

Would that be possible? Or could that maybe be a future improvement?

@timokoesters
Copy link
Member

I think we assume that all events that could be requested from the closure are already in a database, so the closure will not ask other servers over federation

crates/ruma-state-res/src/lib.rs Outdated Show resolved Hide resolved
crates/ruma-state-res/src/lib.rs Outdated Show resolved Hide resolved
@DevinR528
Copy link
Member Author

@jplatte I should point this PR at next right, there are definitely breaking changes.

@DevinR528 DevinR528 force-pushed the state-closure branch 2 times, most recently from 3a6106f to 4d496a8 Compare June 30, 2021 12:11
@jplatte
Copy link
Member

jplatte commented Jul 1, 2021

@DevinR528 Yes, all breaking changes should go to next (with small exceptions right before publication of breaking-change releases).

I also agree with the concern about batching, not because of federation but because you would ideally scan the server's DB for all missing events once, rather than scan for single entries one at a time.

That said since Timo approved this and probably discussed it with Devin before this PR was created, I'm okay with merging this under the assumption that it's better than what we have now.

@DevinR528 DevinR528 requested a review from iinuwa as a code owner July 1, 2021 10:41
@DevinR528 DevinR528 changed the base branch from main to next July 1, 2021 11:01
state-res: Remove event_map arg from all functions

state-res: Remove get_or_load_event helper func and fix resolve docs
@DevinR528
Copy link
Member Author

I squashed and added a changelog entry as soon as CI goes green I'll rebase.

@DevinR528 DevinR528 merged commit 1745558 into ruma:next Jul 1, 2021
@DevinR528 DevinR528 deleted the state-closure branch July 1, 2021 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants