Fix: better (Redis) exception handling #89
Conversation
lib/logstash/inputs/redis.rb
Outdated
| Stud.stoppable_sleep(1) { stop? } | ||
| retry if !stop? | ||
| rescue => e | ||
| retry unless handle_error(e) |
There was a problem hiding this comment.
A suggestion to rename handle_error
I can understand you want to retry when handle_error cannot handle, meaning return false
Looking at the whole function, we actually want to retry when it is retryable error, somehow meaning we can handle it, so it is a bit confusing at the first look.
may be we can do retry handle_error(e) or retry retryable_error(e) ?
There was a problem hiding this comment.
yeah, no super clear - also struggled a bit with naming here.
we're trying to re-use this method as much as possible.
so handle_error returns true if error is handled and should not be retried 😉
I guess you meant retry if handle_error(e),
that is actually what I started with but the inverse boolean condition confused someone else 🤣
There was a problem hiding this comment.
I think retry unless handle_error is ok - another way you could possibly handle this is to just have the new method a no side effect logging method, like log_error(e), which just deals with the formatting/logging keeping the actual retry logic (with the ShutdownSignal special case) local
There was a problem hiding this comment.
thanks, split the method into two.
still needed some kind of retry if ... as the logic is at two places - did not want to have the same logic inline twice. esp. since looking at the plugin the first time I manage to miss the "other" error handling place initially.
let's see what @kaisecheng thinks 🤞
There was a problem hiding this comment.
This is much better. Splitting to two is great 👏 . The naming and comment help a lot. Thanks for doing it.
lib/logstash/inputs/redis.rb
Outdated
| end | ||
| else | ||
| @redis.disconnect! | ||
| redis.disconnect! |
There was a problem hiding this comment.
This commit removes this comment:
if its a SubscribedClient then:
# it does not have a disconnect method (yet)
It feels like there is some context missing as to why disconnect! is not called when redis is subscribed, if this is indeed still the desired behaviour.
There was a problem hiding this comment.
believe this was a left over from the Redis 3.x days.
Redis#disconnect is always fine to call (in 4.x) regardless of the @client used (@original_client is never "replaced").
also no reason not to have disconnect (close) here, I've updated the PR - thanks for the hint.
lib/logstash/inputs/redis.rb
Outdated
| Stud.stoppable_sleep(1) { stop? } | ||
| retry if !stop? | ||
| rescue => e | ||
| retry unless handle_error(e) |
There was a problem hiding this comment.
I think retry unless handle_error is ok - another way you could possibly handle this is to just have the new method a no side effect logging method, like log_error(e), which just deals with the formatting/logging keeping the actual retry logic (with the ShutdownSignal special case) local
Redis doesn't wrap all of the errors e.g. (retriable)
IOErrormight be raised from an IO write (#88).With the refactored error handling we make sure these are logged and retried instead of causing pipeline crashes.
Also not all specs from the suite were run on the CI, as some are tagged with
redis.These require a real Redis server, no reason not to run them against CI as well.
NOTE: extracted from #87 except the problematic subscription 'interrupting' bits (which are still pending)
resolves #88