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

Allow configuration of Synapse's cache without using synctl or environment variables #6391

Merged
merged 66 commits into from
May 11, 2020

Conversation

hawkowl
Copy link
Contributor

@hawkowl hawkowl commented Nov 20, 2019

This is useful in docker environments, where you have the ability to easily change config files, not not always env vars. It also makes it not a weird odd-one-out as far as configuring your Synapse install.

@hawkowl hawkowl assigned hawkowl and unassigned hawkowl Nov 20, 2019
@hawkowl hawkowl requested a review from a team November 20, 2019 17:06
@hawkowl hawkowl force-pushed the hawkowl/cache-config-without-synctl branch from 7f59798 to a14831d Compare November 20, 2019 19:32
@@ -241,7 +240,8 @@ def __getattr__(_self, attr):
# tends to do so in batches, so we need to allow the pool to keep
# lots of idle connections around.
pool = HTTPConnectionPool(self.reactor)
pool.maxPersistentPerHost = max((100 * CACHE_SIZE_FACTOR, 5))
# XXX: Why does this use the cache factor????
Copy link
Member

Choose a reason for hiding this comment

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

it was just a handy way of making it scale for larger instances. The logic went that if you're running a larger instance, you'll bump your cache factor, and will probably want a bigger connection pool too. it's a bit of a hack, but otoh I didn't want to have millions of options that you have to tune.

@richvdh richvdh requested a review from a team November 28, 2019 09:57
synapse/util/caches/lrucache.py Show resolved Hide resolved
synapse/server.py Outdated Show resolved Hide resolved
synapse/util/caches/stream_change_cache.py Show resolved Hide resolved
self._state_group_members_cache = DictionaryCache(
"*stateGroupMembersCache*",
500000 * get_cache_factor_for("stateGroupMembersCache"),
"*stateGroupMembersCache*", 500000
Copy link
Member

Choose a reason for hiding this comment

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

Why does this no longer have a cache factor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because now DictionaryCache does the cache factor math, not the things that call it

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I'm not following where its doing that? AFAICT get_cache_factor_for is only called for a single cache?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, but now you can configure any of the cache sizes, so it's done in the caches itself

synapse/config/cache.py Outdated Show resolved Hide resolved
synapse/util/caches/descriptors.py Show resolved Hide resolved
richvdh
richvdh previously requested changes May 1, 2020
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.

sorry I'm still banging on about sample config.

docs/sample_config.yaml Outdated Show resolved Hide resolved
docs/sample_config.yaml Outdated Show resolved Hide resolved
docs/sample_config.yaml Outdated Show resolved Hide resolved
Comment on lines 1983 to 1984
# Controls the global cache factor. This overrides the "SYNAPSE_CACHE_FACTOR"
# environment variable.
Copy link
Member

Choose a reason for hiding this comment

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

please can you explain what a global cache factor is, and document the default setting?

Copy link
Member

Choose a reason for hiding this comment

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

I've attempted to do so.

@@ -1973,3 +1973,20 @@ opentracing:
#
# logging:
# false


## Cache Configuration ##
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised the very end of the file is the most appropriate place?

Copy link
Member

Choose a reason for hiding this comment

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

I've moved it much further up, near the database config. What do you think?

docs/sample_config.yaml Outdated Show resolved Hide resolved
@anoadragon453 anoadragon453 requested a review from richvdh May 4, 2020 13:17
…cache-config-without-synctl

* 'develop' of github.com:matrix-org/synapse: (43 commits)
  Remove unused store method get_hosts_in_room (#7448)
  Don't UPGRADE database rows
  RST indenting
  Put rollback instructions in upgrade notes
  Fix changelog typo
  Oh yeah, RST
  Absolute URL it is then
  Fix upgrade notes link
  Provide summary of upgrade issues in changelog. Fix )
  Move next version notes from changelog to upgrade notes
  Changelog fixes
  1.13.0rc1
  Documentation on setting up redis (#7446)
  Rework UI Auth session validation for registration (#7455)
  Extend spam checker to allow for multiple modules (#7435)
  Implement OpenID Connect-based login (#7256)
  Add room details admin endpoint (#7317)
  Fix errors from malformed log line (#7454)
  Drop support for redis.dbid (#7450)
  Fixes typo (bellow -> below) (#7449)
  ...
@anoadragon453 anoadragon453 merged commit 7cb8b4b into develop May 11, 2020
@anoadragon453 anoadragon453 deleted the hawkowl/cache-config-without-synctl branch May 11, 2020 17:45
anoadragon453 pushed a commit that referenced this pull request May 14, 2020
anoadragon453 pushed a commit that referenced this pull request May 19, 2020
erikjohnston added a commit that referenced this pull request May 27, 2020
This mostly applise to `*stateGroupCache*` and co.

Broke in #6391.
erikjohnston added a commit that referenced this pull request May 27, 2020
This mostly applise to `*stateGroupCache*` and co.

Broke in #6391.
phil-flex pushed a commit to phil-flex/synapse that referenced this pull request Jun 16, 2020
phil-flex pushed a commit to phil-flex/synapse that referenced this pull request Jun 16, 2020
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.

5 participants