Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Track and deduplicate in-flight requests to _get_state_for_groups. #10870

Merged
merged 36 commits into from
Feb 18, 2022

Conversation

reivilibre
Copy link
Contributor

@reivilibre reivilibre commented Sep 21, 2021

This is the 2nd PR of a series (expected to have 4 PRs) to address #10301 .

Depends on #10825.

The general idea is that when requesting some state, we make use of existing queries that are already in-flight.
Those queries might only give us partial information, so we may have to use multiple, and we may even have to spawn our own (smaller) query for the leftover state.

Coming up later:

  • ordering the in-flight request cache so that 'wider' StateFilters get gathered first
  • capping the number of in-flight requests for a group, before we just grab all the state at once and satisfy everyone.

@reivilibre reivilibre requested review from erikjohnston and removed request for erikjohnston September 21, 2021 16:00
@reivilibre
Copy link
Contributor Author

I'm going to add some more tests when #10825 is available so I can actually prove that things get deduplicated, but would like some thoughts on if this seems sensible.

This reverts commit 363565e
because a proper implementation is now in develop.
@reivilibre reivilibre requested a review from a team as a code owner October 12, 2021 09:48
synapse/storage/databases/state/store.py Outdated Show resolved Hide resolved
synapse/storage/databases/state/store.py Outdated Show resolved Hide resolved
synapse/storage/databases/state/store.py Outdated Show resolved Hide resolved
synapse/storage/databases/state/store.py Outdated Show resolved Hide resolved
synapse/storage/databases/state/store.py Outdated Show resolved Hide resolved
synapse/storage/databases/state/store.py Outdated Show resolved Hide resolved
@erikjohnston erikjohnston added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Nov 1, 2021

d.callback(result)

def test_duplicate_requests_deduplicated(self) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test seems nuanced enough that it could use a docstring saying what the overall steps are that are happening.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added one; any good?

Copy link
Contributor

@clokep clokep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable, thank you for the explanations!

Copy link
Contributor

@clokep clokep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes seem good! 👍 This has a failing sytest which I don't think I've seen before though.

@reivilibre reivilibre merged commit 284ea20 into develop Feb 18, 2022
@reivilibre reivilibre deleted the rei/gsfg_1 branch February 18, 2022 17:23
reivilibre added a commit that referenced this pull request Mar 1, 2022
reivilibre added a commit that referenced this pull request Mar 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants