From d3a6e38c96d95f5c6fa1dc41401ad09d293c218c Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 4 May 2021 10:40:18 +0100 Subject: Apply suggestions from code review Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- synapse/config/server.py | 2 +- synapse/metrics/__init__.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/config/server.py b/synapse/config/server.py index ca1c9711f8..e95925d1ab 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -924,7 +924,7 @@ class ServerConfig(Config): # A value of `[1, 10, 30]` indicates that a second must pass between consecutive # generation 0 GCs, etc. # - # gc_min_seconds_between: [1, 10, 30] + #gc_min_seconds_between: [1, 10, 30] # Set the limit on the returned events in the timeline in the get # and sync operations. The default value is 100. -1 means no upper limit. diff --git a/synapse/metrics/__init__.py b/synapse/metrics/__init__.py index 6de91d826c..93662fa134 100644 --- a/synapse/metrics/__init__.py +++ b/synapse/metrics/__init__.py @@ -539,7 +539,7 @@ REGISTRY.register(ReactorLastSeenMetric()) # thresholds and counts. MIN_TIME_BETWEEN_GCS = [1, 10, 30] -# The time in seconds of the last time we did a GC for each generation. +# The time (in seconds since the epoch) of the last time we did a GC for each generation. _last_gc = [0, 0, 0] -- cgit 1.5.1 From bd04fb63085bc46282e88bad3296b1225425fd4d Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 4 May 2021 10:47:32 +0100 Subject: Code review --- synapse/config/server.py | 23 ++++++++++++++++++++--- synapse/metrics/__init__.py | 2 +- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/synapse/config/server.py b/synapse/config/server.py index e95925d1ab..b639bd3144 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -572,7 +572,7 @@ class ServerConfig(Config): _warn_if_webclient_configured(self.listeners) self.gc_thresholds = read_gc_thresholds(config.get("gc_thresholds", None)) - self.gc_seconds = read_gc_thresholds(config.get("gc_min_seconds_between", None)) + self.gc_seconds = self.read_gc_intervals(config.get("gc_min_interval", None)) @attr.s class LimitRemoteRoomsConfig: @@ -921,10 +921,10 @@ class ServerConfig(Config): # The minimum time in seconds between each GC for a generation, regardless of # the GC thresholds. This ensures that we don't do GC too frequently. # - # A value of `[1, 10, 30]` indicates that a second must pass between consecutive + # A value of `[1s, 10s, 30s]` indicates that a second must pass between consecutive # generation 0 GCs, etc. # - #gc_min_seconds_between: [1, 10, 30] + #gc_min_interval: [0.5s, 30s, 1m] # Set the limit on the returned events in the timeline in the get # and sync operations. The default value is 100. -1 means no upper limit. @@ -1314,6 +1314,23 @@ class ServerConfig(Config): help="Turn on the twisted telnet manhole service on the given port.", ) + def read_gc_intervals(self, durations): + """Reads the three durations for the GC min interval option.""" + if durations is None: + return None + try: + if len(durations) != 3: + raise ValueError() + return ( + self.parse_duration(durations[0]), + self.parse_duration(durations[1]), + self.parse_duration(durations[2]), + ) + except Exception: + raise ConfigError( + "Value of `gc_min_interval` must be a list of three durations if set" + ) + def is_threepid_reserved(reserved_threepids, threepid): """Check the threepid against the reserved threepid config diff --git a/synapse/metrics/__init__.py b/synapse/metrics/__init__.py index 93662fa134..149ae3002a 100644 --- a/synapse/metrics/__init__.py +++ b/synapse/metrics/__init__.py @@ -537,7 +537,7 @@ REGISTRY.register(ReactorLastSeenMetric()) # The minimum time in seconds between GCs for each generation, regardless of the current GC # thresholds and counts. -MIN_TIME_BETWEEN_GCS = [1, 10, 30] +MIN_TIME_BETWEEN_GCS = (1, 10, 30) # The time (in seconds since the epoch) of the last time we did a GC for each generation. _last_gc = [0, 0, 0] -- cgit 1.5.1 From 43c9acda4c873d2f3b0872509ea0431d3e5ed1ff Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 4 May 2021 11:49:13 +0100 Subject: Config --- docs/sample_config.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index ebf364cf40..94b28fecaf 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -155,10 +155,10 @@ presence: # The minimum time in seconds between each GC for a generation, regardless of # the GC thresholds. This ensures that we don't do GC too frequently. # -# A value of `[1, 10, 30]` indicates that a second must pass between consecutive +# A value of `[1s, 10s, 30s]` indicates that a second must pass between consecutive # generation 0 GCs, etc. # -# gc_min_seconds_between: [1, 10, 30] +#gc_min_interval: [0.5s, 30s, 1m] # Set the limit on the returned events in the timeline in the get # and sync operations. The default value is 100. -1 means no upper limit. -- cgit 1.5.1 From b5169b68e90fe494d0137e4676af929b85594a50 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 4 May 2021 14:23:02 +0100 Subject: Document default. Add type annotations. Correctly convert to seconds --- docs/sample_config.yaml | 2 ++ synapse/config/server.py | 15 +++++++++------ synapse/metrics/__init__.py | 6 +++--- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 94b28fecaf..a1dacefbd7 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -158,6 +158,8 @@ presence: # A value of `[1s, 10s, 30s]` indicates that a second must pass between consecutive # generation 0 GCs, etc. # +# Defaults to `[1s, 10s, 30s]`. +# #gc_min_interval: [0.5s, 30s, 1m] # Set the limit on the returned events in the timeline in the get diff --git a/synapse/config/server.py b/synapse/config/server.py index b639bd3144..c290a35a92 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -19,7 +19,7 @@ import logging import os.path import re from textwrap import indent -from typing import Any, Dict, Iterable, List, Optional, Set +from typing import Any, Dict, Iterable, List, Optional, Set, Tuple import attr import yaml @@ -924,6 +924,8 @@ class ServerConfig(Config): # A value of `[1s, 10s, 30s]` indicates that a second must pass between consecutive # generation 0 GCs, etc. # + # Defaults to `[1s, 10s, 30s]`. + # #gc_min_interval: [0.5s, 30s, 1m] # Set the limit on the returned events in the timeline in the get @@ -1314,17 +1316,18 @@ class ServerConfig(Config): help="Turn on the twisted telnet manhole service on the given port.", ) - def read_gc_intervals(self, durations): - """Reads the three durations for the GC min interval option.""" + def read_gc_intervals(self, durations) -> Optional[Tuple[float, float, float]]: + """Reads the three durations for the GC min interval option, returning seconds.""" if durations is None: return None + try: if len(durations) != 3: raise ValueError() return ( - self.parse_duration(durations[0]), - self.parse_duration(durations[1]), - self.parse_duration(durations[2]), + self.parse_duration(durations[0]) / 1000, + self.parse_duration(durations[1]) / 1000, + self.parse_duration(durations[2]) / 1000, ) except Exception: raise ConfigError( diff --git a/synapse/metrics/__init__.py b/synapse/metrics/__init__.py index 149ae3002a..e671da26d5 100644 --- a/synapse/metrics/__init__.py +++ b/synapse/metrics/__init__.py @@ -537,10 +537,10 @@ REGISTRY.register(ReactorLastSeenMetric()) # The minimum time in seconds between GCs for each generation, regardless of the current GC # thresholds and counts. -MIN_TIME_BETWEEN_GCS = (1, 10, 30) +MIN_TIME_BETWEEN_GCS = (1.0, 10.0, 30.0) # The time (in seconds since the epoch) of the last time we did a GC for each generation. -_last_gc = [0, 0, 0] +_last_gc = [0.0, 0.0, 0.0] def runUntilCurrentTimer(reactor, func): @@ -601,7 +601,7 @@ def runUntilCurrentTimer(reactor, func): unreachable = gc.collect(i) end = time.time() - _last_gc[i] = int(end) + _last_gc[i] = end gc_time.labels(i).observe(end - start) gc_unreachable.labels(i).set(unreachable) -- cgit 1.5.1