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

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3a9f5bf
Don't fetch state for missing events that we fetched
erikjohnston Apr 28, 2017
9b147cd
Remove unncessary call in _get_missing_events_for_pdu
erikjohnston Apr 28, 2017
2347efc
Fixup
erikjohnston Apr 28, 2017
ef86218
Merge together redundant calculations/logging
erikjohnston May 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions synapse/handlers/federation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,23 @@ def on_receive_pdu(self, origin, pdu, get_missing=True):
yield self._get_missing_events_for_pdu(
origin, pdu, prevs, min_depth
)

# 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?

)

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?

if prevs - seen:
logger.info(
"Still missing %d prev events for %s: %r...",
len(prevs - seen), pdu.event_id, list(prevs - seen)[:5]
)
else:
logger.info(
"Found all missing prev events for %s", pdu.event_id
)
elif prevs - seen:
logger.info(
"Not fetching %d missing events for room %r,event %s: %r...",
Expand Down Expand Up @@ -288,19 +305,6 @@ def _get_missing_events_for_pdu(self, origin, pdu, prevs, min_depth):
get_missing=False
)

have_seen = yield self.store.have_events(
[ev for ev, _ in pdu.prev_events]
)
seen = set(have_seen.keys())
if prevs - seen:
logger.info(
"Still missing %d prev events for %s: %r...",
len(prevs - seen), pdu.event_id, list(prevs - seen)[:5]
)
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


@log_function
@defer.inlineCallbacks
def _process_received_pdu(self, origin, pdu, state, auth_chain):
Expand Down