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

Fix tests on postgresql#3740

Merged
hawkowl merged 28 commits into
developfrom
hawkowl/pgtests
Sep 3, 2018
Merged

Fix tests on postgresql#3740
hawkowl merged 28 commits into
developfrom
hawkowl/pgtests

Conversation

@hawkowl
Copy link
Copy Markdown
Contributor

@hawkowl hawkowl commented Aug 22, 2018

No description provided.

@hawkowl hawkowl requested a review from a team August 31, 2018 13:10
@hawkowl hawkowl changed the title [WIP] Fix tests on postgresql Fix tests on postgresql Aug 31, 2018
Comment thread synapse/storage/monthly_active_users.py Outdated
allow_none=True,
desc="user_last_seen_monthly_active",
))
defer.returnValue(res)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the reason for converting all these functions to defer.inlineCallbacks?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

me trying things and inadvertently committing it :) -- reverted

Comment thread synapse/storage/_base.py Outdated
)
self.clock = self.hs.get_clock()
else:
self._cache_id_gen = None
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So, fun fact: this now gets set on the slave replication stores as well as the store on master. This is a bit awkward because we do set self._cache_id_gen on the replication stores in synapse/replication/slave/storage/_base.py to something different.

In short, everything is awful and is scary and I'm scared about moving this here in case the replication stuff fail to properly overwrite this definition

Copy link
Copy Markdown
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 legit. I've not gone through all the test changes, but they seem sane.

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