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…
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