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

Discard RDATA from already seen positions #7648

Merged
merged 18 commits into from
Jun 15, 2020
Merged

Conversation

clokep
Copy link
Member

@clokep clokep commented Jun 5, 2020

When processing RDATA commands via replication, discard any RDATA that has already been seen. This is calculated via asking the stream to get the "current position" and comparing to the incoming token.

Fixes #7360

To Do

  • Handle streams that can reset (e.g. typing and federation).
  • Add tests for resetting the above.

@clokep clokep changed the title Discard RDATA from seen positions Discard RDATA from already seen positions Jun 5, 2020
@clokep clokep requested a review from erikjohnston June 8, 2020 14:59
@clokep
Copy link
Member Author

clokep commented Jun 8, 2020

@erikjohnston Hope it is OK to set you directly as the reviewer, but since we've already talked about it a bunch that seemed to make the most sense.

This approach seems to work fine for streams that reset, with our assumption that a POSITION is sent before any RDATA once it is reset. Unfortunately that assumption isn't really asserted (or even explained) anywhere.

Please let me know how this looks!

Comment on lines 743 to 744
if stream_name == TypingStream.NAME:
self.typing_handler.process_replication_rows(token, [])
Copy link
Member Author

Choose a reason for hiding this comment

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

This will need to be expanded to include additional streams, if it is the right fix.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think this makes sense. Though I wonder if we should just call on_rdata so that we don't duplicate things

@clokep clokep requested a review from erikjohnston June 9, 2020 16:57
@clokep
Copy link
Member Author

clokep commented Jun 9, 2020

@erikjohnston Can you take a look at this again?

Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

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

This looks good! The gut wrenching in the typing test is a bit sad, but I don't suggest we try and clean it up now

Comment on lines 743 to 744
if stream_name == TypingStream.NAME:
self.typing_handler.process_replication_rows(token, [])
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think this makes sense. Though I wonder if we should just call on_rdata so that we don't duplicate things

synapse/replication/tcp/handler.py Outdated Show resolved Hide resolved
synapse/replication/tcp/handler.py Outdated Show resolved Hide resolved
tests/replication/tcp/streams/test_typing.py Show resolved Hide resolved
@clokep clokep marked this pull request as ready for review June 11, 2020 12:34
@@ -738,6 +738,10 @@ async def _process_and_notify(self, stream_name, instance_name, token, rows):
except Exception:
logger.exception("Error processing replication")

async def on_position(self, stream_name: str, instance_name: str, token: int):
await super().on_position(stream_name, instance_name, token)
await self.on_rdata(stream_name, instance_name, token, [])
Copy link
Member Author

Choose a reason for hiding this comment

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

This probably needs a comment, but I'm not really sure what to suggest...

Also call on_rdata to ensure that stream positions are properly reset.

Or something?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, something like that. I don't have any better suggestions at least :/

@clokep clokep requested a review from erikjohnston June 11, 2020 12:35
@clokep clokep merged commit 7d2532b into develop Jun 15, 2020
@clokep clokep deleted the clokep/discard-old-rdata branch June 15, 2020 12:44
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* commit '03619324f':
  Create a ListenerConfig object (#7681)
  Fix changelog wording
  1.15.1
  Wrap register_device coroutine in an ensureDeferred (#7684)
  Ensure the body is a string before comparing push rules. (#7701)
  Ensure etag is a string for GET room_keys/version response (#7691)
  Update m.id.phone to use 'phone' instead of 'number' (#7687)
  Fix "There was no active span when trying to log." error (#7698)
  Enable 3PID add/bind/unbind endpoints on r0 routes
  Discard RDATA from already seen positions. (#7648)
  Replace iteritems/itervalues/iterkeys with native versions. (#7692)
  Fix warnings about losing log context during UI auth. (#7688)
  Fix a typo when comparing the URI & method during UI Auth. (#7689)
  Remove "user_id" from GET /presence. (#7606)
  Increase the default SAML session expirary time to 15 minutes. (#7664)
  fix typo in sample_config.yaml (#7652)
  Take out a lock before modifying _CACHES (#7663)
  Add option to enable encryption by default for new rooms (#7639)
  Clean-up the fallback login code. (#7657)
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.

it's possible for stream_ids to go backwards in a replication stream
2 participants