Bail early if remote wouldn't be retried #2064
Conversation
synapse/util/retryutils.py
Outdated
|
|
||
| if retry_last_ts + retry_interval > now: | ||
| defer.returnValue(True) | ||
| defer.returnValue(True) |
There was a problem hiding this comment.
This appears to be missing a False case.
synapse/util/retryutils.py
Outdated
| @@ -35,6 +35,29 @@ def __init__(self, retry_last_ts, retry_interval, destination): | |||
|
|
|||
|
|
|||
| @defer.inlineCallbacks | |||
| def would_retry(destination, clock, store): | |||
There was a problem hiding this comment.
I feel like this is a bit of a funny name, when it is equally applicable to the very first message we send to a destination (so in no way a 'retry'). Maybe would_send would be better? Or would_throw_on_send (and invert the result). Neither is particularly great though - I don't mind if you want to leave it.
There was a problem hiding this comment.
Of course another way to do this is just to call get_retry_limiter, and throw away the result, and see if you get an exception.
synapse/util/retryutils.py
Outdated
| def would_retry(destination, clock, store): | ||
| """Check if ``get_retry_limiter`` would throw. This is purely to allow | ||
| things to bail early if the resulting request would not otherwise be sent | ||
| """ |
There was a problem hiding this comment.
please can we document types of args and return values, even if it's "obvious". I'm fed up with figuring it all out from first principles in the synapse codebase. You can even c&p most of it from get_retry_limiter in this case.
| try: | ||
| self.pending_transactions[destination] = 1 | ||
|
|
||
| if not (yield would_retry(destination, self.clock, self.store)): |
There was a problem hiding this comment.
given we logger.debug when we get a NotRetryingDestination exception (a block of code which is now almost redundant, btw), should we do so here? should we in fact throw a NotRetryingDestination here?
|
You raise excellent points. |
| try: | ||
| self.pending_transactions[destination] = 1 | ||
|
|
||
| # This will throw if we woldn't retry. We do this here so we fail |
No description provided.