Revert "Allow for the configuration of max request retries and min/max retry delays in the matrix federation client (#12504)"

This reverts commit d84e66144d.
This commit is contained in:
Mathieu Velten 2023-06-14 11:55:09 +02:00
parent 14f9d9b452
commit ef0d3d7bd9
5 changed files with 10 additions and 68 deletions

View File

@ -30,7 +30,6 @@ Improved Documentation
Internal Changes Internal Changes
---------------- ----------------
- Allow for the configuration of max request retries and min/max retry delays in the matrix federation client. ([\#12504](https://github.com/matrix-org/synapse/issues/12504))
- Log when events are (maybe unexpectedly) filtered out of responses in tests. ([\#14213](https://github.com/matrix-org/synapse/issues/14213)) - Log when events are (maybe unexpectedly) filtered out of responses in tests. ([\#14213](https://github.com/matrix-org/synapse/issues/14213))
- Read from column `full_user_id` rather than `user_id` of tables `profiles` and `user_filters`. ([\#15649](https://github.com/matrix-org/synapse/issues/15649)) - Read from column `full_user_id` rather than `user_id` of tables `profiles` and `user_filters`. ([\#15649](https://github.com/matrix-org/synapse/issues/15649))
- Add support for tracing functions which return `Awaitable`s. ([\#15650](https://github.com/matrix-org/synapse/issues/15650)) - Add support for tracing functions which return `Awaitable`s. ([\#15650](https://github.com/matrix-org/synapse/issues/15650))

View File

@ -1196,32 +1196,6 @@ Example configuration:
allow_device_name_lookup_over_federation: true allow_device_name_lookup_over_federation: true
``` ```
--- ---
### `federation`
The federation section defines some sub-options related to federation.
The following options are related to configuring timeout and retry logic for one request,
independently of the others.
Short retry algorithm is used when something or someone will wait for the request to have an
answer, while long retry is used for requests that happen in the background,
like sending a federation transaction.
* `client_timeout`: timeout for the federation requests in seconds. Default to 60s.
* `max_short_retry_delay`: maximum delay to be used for the short retry algo in seconds. Default to 2s.
* `max_long_retry_delay`: maximum delay to be used for the short retry algo in seconds. Default to 60s.
* `max_short_retries`: maximum number of retries for the short retry algo. Default to 3 attempts.
* `max_long_retries`: maximum number of retries for the long retry algo. Default to 10 attempts.
Example configuration:
```yaml
federation:
client_timeout: 180
max_short_retry_delay: 7
max_long_retry_delay: 100
max_short_retries: 5
max_long_retries: 20
```
---
## Caching ## Caching
Options related to caching. Options related to caching.

View File

@ -22,8 +22,6 @@ class FederationConfig(Config):
section = "federation" section = "federation"
def read_config(self, config: JsonDict, **kwargs: Any) -> None: def read_config(self, config: JsonDict, **kwargs: Any) -> None:
federation_config = config.setdefault("federation", {})
# FIXME: federation_domain_whitelist needs sytests # FIXME: federation_domain_whitelist needs sytests
self.federation_domain_whitelist: Optional[dict] = None self.federation_domain_whitelist: Optional[dict] = None
federation_domain_whitelist = config.get("federation_domain_whitelist", None) federation_domain_whitelist = config.get("federation_domain_whitelist", None)
@ -51,13 +49,5 @@ class FederationConfig(Config):
"allow_device_name_lookup_over_federation", False "allow_device_name_lookup_over_federation", False
) )
# Allow for the configuration of timeout, max request retries
# and min/max retry delays in the matrix federation client.
self.client_timeout = federation_config.get("client_timeout", 60)
self.max_long_retry_delay = federation_config.get("max_long_retry_delay", 60)
self.max_short_retry_delay = federation_config.get("max_short_retry_delay", 2)
self.max_long_retries = federation_config.get("max_long_retries", 10)
self.max_short_retries = federation_config.get("max_short_retries", 3)
_METRICS_FOR_DOMAINS_SCHEMA = {"type": "array", "items": {"type": "string"}} _METRICS_FOR_DOMAINS_SCHEMA = {"type": "array", "items": {"type": "string"}}

View File

@ -95,6 +95,8 @@ incoming_responses_counter = Counter(
) )
MAX_LONG_RETRIES = 10
MAX_SHORT_RETRIES = 3
MAXINT = sys.maxsize MAXINT = sys.maxsize
@ -404,12 +406,7 @@ class MatrixFederationHttpClient:
self.clock = hs.get_clock() self.clock = hs.get_clock()
self._store = hs.get_datastores().main self._store = hs.get_datastores().main
self.version_string_bytes = hs.version_string.encode("ascii") self.version_string_bytes = hs.version_string.encode("ascii")
self.default_timeout = hs.config.federation.client_timeout self.default_timeout = 60
self.max_long_retry_delay = hs.config.federation.max_long_retry_delay
self.max_short_retry_delay = hs.config.federation.max_short_retry_delay
self.max_long_retries = hs.config.federation.max_long_retries
self.max_short_retries = hs.config.federation.max_short_retries
self._cooperator = Cooperator(scheduler=_make_scheduler(self.reactor)) self._cooperator = Cooperator(scheduler=_make_scheduler(self.reactor))
@ -586,9 +583,9 @@ class MatrixFederationHttpClient:
# XXX: Would be much nicer to retry only at the transaction-layer # XXX: Would be much nicer to retry only at the transaction-layer
# (once we have reliable transactions in place) # (once we have reliable transactions in place)
if long_retries: if long_retries:
retries_left = self.max_long_retries retries_left = MAX_LONG_RETRIES
else: else:
retries_left = self.max_short_retries retries_left = MAX_SHORT_RETRIES
url_bytes = request.uri url_bytes = request.uri
url_str = url_bytes.decode("ascii") url_str = url_bytes.decode("ascii")
@ -733,12 +730,12 @@ class MatrixFederationHttpClient:
if retries_left and not timeout: if retries_left and not timeout:
if long_retries: if long_retries:
delay = 4 ** (self.max_long_retries + 1 - retries_left) delay = 4 ** (MAX_LONG_RETRIES + 1 - retries_left)
delay = min(delay, self.max_long_retry_delay) delay = min(delay, 60)
delay *= random.uniform(0.8, 1.4) delay *= random.uniform(0.8, 1.4)
else: else:
delay = 0.5 * 2 ** (self.max_short_retries - retries_left) delay = 0.5 * 2 ** (MAX_SHORT_RETRIES - retries_left)
delay = min(delay, self.max_short_retry_delay) delay = min(delay, 2)
delay *= random.uniform(0.8, 1.4) delay *= random.uniform(0.8, 1.4)
logger.debug( logger.debug(

View File

@ -40,7 +40,7 @@ from synapse.server import HomeServer
from synapse.util import Clock from synapse.util import Clock
from tests.server import FakeTransport from tests.server import FakeTransport
from tests.unittest import HomeserverTestCase, override_config from tests.unittest import HomeserverTestCase
def check_logcontext(context: LoggingContextOrSentinel) -> None: def check_logcontext(context: LoggingContextOrSentinel) -> None:
@ -640,21 +640,3 @@ class FederationClientTests(HomeserverTestCase):
self.cl.build_auth_headers( self.cl.build_auth_headers(
b"", b"GET", b"https://example.com", destination_is=b"" b"", b"GET", b"https://example.com", destination_is=b""
) )
@override_config(
{
"federation": {
"client_timeout": 180,
"max_long_retry_delay": 100,
"max_short_retry_delay": 7,
"max_long_retries": 20,
"max_short_retries": 5,
}
}
)
def test_configurable_retry_and_delay_values(self) -> None:
self.assertEqual(self.cl.default_timeout, 180)
self.assertEqual(self.cl.max_long_retry_delay, 100)
self.assertEqual(self.cl.max_short_retry_delay, 7)
self.assertEqual(self.cl.max_long_retries, 20)
self.assertEqual(self.cl.max_short_retries, 5)