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

Refactor getting replication updates from database v2. #7740

Merged
merged 13 commits into from
Jul 7, 2020

Conversation

erikjohnston
Copy link
Member

This is a follow on from #7636 to do most of the other streams. There are still a few streams left that don't have store functions that follow the new scheme, but those are special snowflakes that have more complicated handling in their stream class. We should probably fix those up to, but I don't think they

c.f. #7340

@erikjohnston erikjohnston requested a review from a team June 24, 2020 17:48
Copy link
Member

@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.

I think this looks OK overall. I think I see why the db_query_to_update_function existed now, as there's a lot of similar, but not quite the same-ish code.

Although this duplicates code a bit, the fewer levels of indirection will be useful, I think! 👍

Note that I kind of skipped some of the comments since they seem to be copied and pasted?

updates = [(row[0], row[1:]) for row in txn]
limited = False
upto_token = current_id
if len(updates) >= limit:
Copy link
Member

Choose a reason for hiding this comment

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

#7636 seems to use a mixture of >, >= and == to determine if the stream was limited. I'm not sure if this inconsistency is on purpose or not. I would have thought > would be the proper choice here to avoid unnecessary work, but maybe that's wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes, sorry. It mostly depends on the SQL query. Some are LIMIT ? and so will only ever return at most limit rows. Some have more fuzzy bounds and so could return more. I don't think anywhere uses >? If they do it sounds like a bug

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, typing uses >, but that is a special case because we have all results in memory and don't query the DB with a limit

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. Is this worth a comment in any of the locations?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a comment to the typing one 👍

synapse/storage/data_stores/main/end_to_end_keys.py Outdated Show resolved Hide resolved
@@ -98,77 +98,69 @@ def get_pushers(txn):
rows = yield self.db.runInteraction("get_all_pushers", get_pushers)
return rows

def get_all_updated_pushers(self, last_id, current_id, limit):
Copy link
Member

Choose a reason for hiding this comment

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

Was get_all_updated_pushers unused?

Copy link
Member Author

Choose a reason for hiding this comment

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

Err, yes it was

@erikjohnston erikjohnston requested a review from clokep July 6, 2020 09:13
Copy link
Member

@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.

There are still a few streams left that don't have store functions that follow the new scheme, but those are special snowflakes that have more complicated handling in their stream class. We should probably fix those up to, but I don't think they

@erikjohnston What was the rest of this final sentence? 😄

I think that this PR looks good though.

@erikjohnston
Copy link
Member Author

How am I so bad at writing? I think that was meant to be:

We should probably fix those up to, but I don't think they are going to confuse anyone too much

@erikjohnston erikjohnston merged commit 67d7756 into develop Jul 7, 2020
@erikjohnston erikjohnston deleted the erikj/move_limited_checks branch July 7, 2020 11:11
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* commit '43726783e': (22 commits)
  1.17.0rc1
  Fix some spelling mistakes / typos. (#7811)
  `update_membership` declaration: now always returns an event id. (#7809)
  Improve stacktraces from exceptions in background processes (#7808)
  Fix `can only concatenate list (not "tuple") to list` exception (#7810)
  Pass original request headers from workers to the main process. (#7797)
  Generate real events when we reject invites (#7804)
  Add `HomeServer.signing_key` property (#7805)
  Revert "Update the installation docs on apt-transport-https (#7801)"
  Do not use simplejson in Synapse. (#7800)
  Stop passing bytes when dumping JSON (#7799)
  Update the installation docs on apt-transport-https (#7801)
  shuffle changelog slightly
  Change Caddy links (old is deprecated) (#7789)
  Stop populating unused table `local_invites`. (#7793)
  Refactor getting replication updates from database v2. (#7740)
  Add libwebp dependency to Dockerfile (#7791)
  Add documentation for JWT login type and improve sample config. (#7776)
  Convert the appservice handler to async/await. (#7775)
  Don't ignore `set_tweak` actions with no explicit `value`. (#7766)
  ...
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.

2 participants