BUGFIX: Add exponential backoff when reading from Redis #3344
Conversation
| do { | ||
| try { | ||
| return $this->uncompress($this->redis->get($this->getPrefixedIdentifier('entry:' . $entryIdentifier))); | ||
| } catch (\RedisException $exception) { |
There was a problem hiding this comment.
Seems dangerous to me.. Isn't there at least a more specific exception that is thrown when "Redis is not ready (yet)"?
There was a problem hiding this comment.
No, you can only inspect the error message.
But then again, even if it's something else, why not try again? What could be dangerous?
There was a problem hiding this comment.
I'm not sure but I've seen so many errors in the past due to silently caught exceptions that those make me suspicious.
Also we have seen some performance issues with exponential back offs in the past (and are replacing those in Neos 9). For example: Some error in Redis will now lead to a 20s+ delay (if I counted correctly) until it is displayed. A wrong configuration could easily build up to kill the server upon deployments.
What about moving this logic to the getStatus() implementation and make sure that it is called upon deployment?
And/or implement the WithSetupInterface and put it there
There was a problem hiding this comment.
Yeah, maybe logging the exceptions would be good – after all, at that point nothing should go wrong.
About killing something on a deployment: The use case for me here is exactly a deployment failure. 🙃
Adding something else to explicitly call won't happen (in "my" current project), as we have that workaround already: Call doctrine:compileproxies after deployment until no error appears… 🙈
There was a problem hiding this comment.
About killing something on a deployment: The use case for me here is exactly a deployment failure.
Right, it fixes the error you described in the issue, but it might create a new one for misconfigured backends.
I won't block this, but I'm not a big fan as you might be able to tell :)
What about turning this into a composition, i.e. introduce some RetryBackend that can be wrapped around any other backend (similar to the MultiBackend)?
There was a problem hiding this comment.
That is also an option…
There was a problem hiding this comment.
I think that the PDO and Redis backend should both have a automatic retry implementation like this by default. In modern hosting environments, there will be movement and it can happen that a Redis is gone for a second or two once in a while. Therefore, it would be nice if a Flow dev wouldn't have to think about this and her application would just work.
I see the point of @bwaidelich that sometimes retries can worsen things. You need to apply the "circuit braker" design pattern for those cases. However, it depends …
I wouldn't like to introduce another wrapper backend since it makes the setup complicated and I think that a reasonable retry is what most people want.
How about making this configurable (on / off and number of retries) using the same option names for RedisBackend, PDOBackend and any other backend which could profit from this?
When Redis is not ready (yet) when trying to read from the cache, this makes Flow wait and retry up to 8 times with an exponentially growing back-off time.
Fixes #3284
Review instructions
This might be hard to check in real life, sorry…
Checklist
FEATURE|TASK|BUGFIX!!!and have upgrade-instructions