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

Don't fetch state for missing events that we fetched #2170

Merged
merged 4 commits into from
May 3, 2017
Merged

Don't fetch state for missing events that we fetched #2170

merged 4 commits into from
May 3, 2017

Conversation

erikjohnston
Copy link
Member

After fetching missing events we didn't update the list of events that were missing, so while we did successfully fetch the missing events it still thought it had a hole. This meant that it fetched the state, which adds delays, CPU time and increased DB usage (since we don't delta encode those state sets)

@richvdh
Copy link
Member

richvdh commented Apr 28, 2017 •
edited
Loading

For the record, this was introduced in synapse 0.20, in 3406333

@erikjohnston: the intention was that _get_missing_events_for_pdu returns the new value for have_seen. It might well make more sense for the call to have_events to be where you have put it, but in that case you can remove the redundant call in _get_missing_events_for_pdu.

@richvdh richvdh assigned erikjohnston and unassigned richvdh Apr 28, 2017
@erikjohnston
Copy link
Member Author

I think its more explicit to have it done in the outer function. WIll remove from _get_missing

@erikjohnston erikjohnston assigned richvdh and unassigned erikjohnston Apr 28, 2017
richvdh
richvdh reviewed Apr 28, 2017
View reviewed changes
synapse/handlers/federation.py
)
else:
logger.info("Found all missing prev events for %s", pdu.event_id)
defer.returnValue(have_seen)
Copy link
Member

Choose a reason for hiding this comment

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

there are other calls to defer.returnValue in this function; please update them too

also please update the docstring.

Copy link
Member

Choose a reason for hiding this comment

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

while you're in there, the docstring claims that prevs should be a list, which looks like a lie. my fault, but could you fix it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

richvdh
richvdh reviewed Apr 28, 2017
View reviewed changes
synapse/handlers/federation.py Outdated
[ev for ev, _ in pdu.prev_events]
)

seen = set(have_seen.keys())
Copy link
Member

Choose a reason for hiding this comment

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

haven't we already got all this at https://github.com/matrix-org/synapse/pull/2170/files#diff-3f6de39b112e0c501c3c2925a571b908R199 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

no?

Copy link
Member

Choose a reason for hiding this comment

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

well, it looks very similar to me. what's the difference?

Copy link
Member Author

Choose a reason for hiding this comment

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

have_seen has been updated the line above, and have_seen needs to be rechecked as we just fetched some missing events. Is that what you mean? Or do you mean we should wrap the call to have_seen and seen in a function?

richvdh
richvdh reviewed Apr 28, 2017
View reviewed changes
synapse/handlers/federation.py Outdated
# Update the set of things we've seen after trying to
# fetch the missing stuff
have_seen = yield self.store.have_events(
[ev for ev, _ in pdu.prev_events]
Copy link
Member

Choose a reason for hiding this comment

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

use prevs here?

@richvdh richvdh assigned erikjohnston and unassigned richvdh Apr 28, 2017
@erikjohnston erikjohnston assigned richvdh and unassigned erikjohnston Apr 28, 2017
richvdh
richvdh reviewed May 2, 2017
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

still unclear why it looks like we're duplicating code here

@richvdh
Copy link
Member

richvdh commented May 2, 2017

(here = https://github.com/matrix-org/synapse/pull/2170/files/9b147cd73049c6222fa6c3b45dfb2a360de93cc4#diff-3f6de39b112e0c501c3c2925a571b908)

@richvdh richvdh assigned erikjohnston and unassigned richvdh May 2, 2017
@erikjohnston erikjohnston assigned richvdh and unassigned erikjohnston May 2, 2017
@erikjohnston
Copy link
Member Author

I am completely failing to understand what you mean I'm afraid :(

@erikjohnston erikjohnston assigned erikjohnston and unassigned richvdh May 3, 2017
@erikjohnston erikjohnston assigned richvdh and unassigned erikjohnston May 3, 2017
richvdh
richvdh approved these changes May 3, 2017
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

this is a horrible, horrible bit of code

but I think it looks ok now

@richvdh richvdh assigned erikjohnston and unassigned richvdh May 3, 2017
@erikjohnston erikjohnston merged commit 0c27383 into develop May 3, 2017
psaavedra added a commit to psaavedra/synapse that referenced this pull request May 19, 2017
@psaavedra
Merge tag 'v0.21.0' into v0.21.0_no_federate_by_default
badd30b 
Changes in synapse v0.21.0 (2017-05-18)
=======================================

No changes since v0.21.0-rc3

Changes in synapse v0.21.0-rc3 (2017-05-17)
===========================================

Features:

* Add per user rate-limiting overrides (PR matrix-org#2208)
* Add config option to limit maximum number of events requested by ``/sync``
  and ``/messages`` (PR matrix-org#2221) Thanks to @psaavedra!

Changes:

* Various small performance fixes (PR matrix-org#2201, matrix-org#2202, matrix-org#2224, matrix-org#2226, matrix-org#2227, matrix-org#2228,
  matrix-org#2229)
* Update username availability checker API (PR matrix-org#2209, matrix-org#2213)
* When purging, don't de-delta state groups we're about to delete (PR matrix-org#2214)
* Documentation to check synapse version (PR matrix-org#2215) Thanks to @hamber-dick!
* Add an index to event_search to speed up purge history API (PR matrix-org#2218)

Bug fixes:

* Fix API to allow clients to upload one-time-keys with new sigs (PR matrix-org#2206)

Changes in synapse v0.21.0-rc2 (2017-05-08)
===========================================

Changes:

* Always mark remotes as up if we receive a signed request from them (PR matrix-org#2190)

Bug fixes:

* Fix bug where users got pushed for rooms they had muted (PR matrix-org#2200)

Changes in synapse v0.21.0-rc1 (2017-05-08)
===========================================

Features:

* Add username availability checker API (PR matrix-org#2183)
* Add read marker API (PR matrix-org#2120)

Changes:

* Enable guest access for the 3pl/3pid APIs (PR matrix-org#1986)
* Add setting to support TURN for guests (PR matrix-org#2011)
* Various performance improvements (PR matrix-org#2075, matrix-org#2076, matrix-org#2080, matrix-org#2083, matrix-org#2108,
  matrix-org#2158, matrix-org#2176, matrix-org#2185)
* Make synctl a bit more user friendly (PR matrix-org#2078, matrix-org#2127) Thanks @APwhitehat!
* Replace HTTP replication with TCP replication (PR matrix-org#2082, matrix-org#2097, matrix-org#2098,
  matrix-org#2099, matrix-org#2103, matrix-org#2014, matrix-org#2016, matrix-org#2115, matrix-org#2116, matrix-org#2117)
* Support authenticated SMTP (PR matrix-org#2102) Thanks @DanielDent!
* Add a counter metric for successfully-sent transactions (PR matrix-org#2121)
* Propagate errors sensibly from proxied IS requests (PR matrix-org#2147)
* Add more granular event send metrics (PR matrix-org#2178)

Bug fixes:

* Fix nuke-room script to work with current schema (PR matrix-org#1927) Thanks
  @zuckschwerdt!
* Fix db port script to not assume postgres tables are in the public schema
  (PR matrix-org#2024) Thanks @jerrykan!
* Fix getting latest device IP for user with no devices (PR matrix-org#2118)
* Fix rejection of invites to unreachable servers (PR matrix-org#2145)
* Fix code for reporting old verify keys in synapse (PR matrix-org#2156)
* Fix invite state to always include all events (PR matrix-org#2163)
* Fix bug where synapse would always fetch state for any missing event (PR matrix-org#2170)
* Fix a leak with timed out HTTP connections (PR matrix-org#2180)
* Fix bug where we didn't time out HTTP requests to ASes  (PR matrix-org#2192)

Docs:

* Clarify doc for SQLite to PostgreSQL port (PR matrix-org#1961) Thanks @benhylau!
* Fix typo in synctl help (PR matrix-org#2107) Thanks @HarHarLinks!
* ``web_client_location`` documentation fix (PR matrix-org#2131) Thanks @matthewjwolff!
* Update README.rst with FreeBSD changes (PR matrix-org#2132) Thanks @feld!
* Clarify setting up metrics (PR matrix-org#2149) Thanks @encks!
@erikjohnston erikjohnston deleted the erikj/fed_hole_state branch October 26, 2017 11:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@richvdh richvdh richvdh approved these changes

Assignees

@erikjohnston erikjohnston

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@erikjohnston @richvdh