-
Notifications
You must be signed in to change notification settings - Fork 76
Log 'RecordTooLargeException' errors as warn #179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
lib/logstash/outputs/kafka.rb
Outdated
| futures.each_with_index do |future, i| | ||
| begin | ||
| result = future.get() | ||
| rescue org.apache.kafka.common.errors.RecordTooLargeException => x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robbavey there's another PR that catches this exception during producer.send
https://github.com/logstash-plugins/logstash-output-kafka/pull/191/files#diff-c6a637e53249d2c5a0ee842b0bc19e82R265 instead of this PR's future.get
I believe this one makes sense as the other operation will only return the future so no exception should be raised, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this PR ups the visibility of an error that was already being logged at debug in the following rescue => e block. I'm kind of tempted to change this PR to just increase the log level of the blanket rescue to increase the visibility of any error caught here, and reduce if the noise levels get too high.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, we want to know whenever an error happens, otherwise logstash will either be silently retrying things or dropping data (Both terrible).
Log errors encountered when sending messages to Kafka at warn, rather than debug, to improve the visibility of errors such as 'RecordTooLargeException.' Fixes logstash-plugins#177
jsvd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending on 💚
Log errors encountered when sending messages to Kafka at warn, rather than debug, to improve the visibility of errors such as 'RecordTooLargeException.' Fixes logstash-plugins#177 Fixes logstash-plugins#179
Fixed incorrect millisecond to second conversion for retry_backoff_ms logstash-plugins#216 Fixed unnecessary sleep after exhausted retries logstash-plugins#166 Changed Kafka send errors to log as warn logstash-plugins#179
Fixed incorrect millisecond to second conversion for retry_backoff_ms logstash-plugins#216 Fixed unnecessary sleep after exhausted retries logstash-plugins#166 Changed Kafka send errors to log as warn logstash-plugins#179
Fixed incorrect millisecond to second conversion for retry_backoff_ms logstash-plugins#216 Fixed unnecessary sleep after exhausted retries logstash-plugins#166 Changed Kafka send errors to log as warn logstash-plugins#179
Fixed unnecessary sleep after exhausted retries logstash-plugins#166 Changed Kafka send errors to log as warn logstash-plugins#179
Fixed unnecessary sleep after exhausted retries logstash-plugins#166 Changed Kafka send errors to log as warn logstash-plugins#179
Fixed unnecessary sleep after exhausted retries logstash-plugins#166 Changed Kafka send errors to log as warn logstash-plugins#179
Fixed unnecessary sleep after exhausted retries logstash-plugins#166 Changed Kafka send errors to log as warn logstash-plugins#179
This is a common error, which cannot be fixed without changing Logstash setting
and should not be hidden as a debug log entry.
Fixes #177