-
Notifications
You must be signed in to change notification settings - Fork 15
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
Leverage the Logstash 8.4.0 DLQ reader to delete consumed segments #43
Leverage the Logstash 8.4.0 DLQ reader to delete consumed segments #43
Conversation
51abb74
to
5d420f8
Compare
5d420f8
to
ec89202
Compare
The 🔴 CI on |
if clean_consumed && !Gem::Requirement.new('>= 8.4.0').satisfied_by?(@logstash_version) | ||
raise ConfigurationError.new("clean_consumed can be used only with Logstash version 8.4.0 and above") | ||
end | ||
if clean_consumed |
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.
We should log or fail the pipeline if we are implicitly changing a configuration setting, particularly one that is explicitly set
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 we could enforce that the user sets also commit_offsets
, I thought about the implicit setting once clean_consumed
to smooth the adoption. But in other plugins we tend to be more explicit, so I'll switch to raising a configuration error.
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.
By default commit_offset
is true
so if the user doesn't explicitly set to false
, the adoption is smooth.
Fixed with 114ab84
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.
A few nits
- Better wording for error. - Fixed RSpec tests to adhere to spec style guide. Co-authored-by: Rob Bavey <rob.bavey@elastic.co>
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
Release notes
Expose the
clean_consumed
configuration setting to remove the consumed DLQ segments.What does this PR do?
Expose the setting to be used in config files and lock the execution to Logstash
>= 8.4.0
because leverages the API provided by PR elastic/logstash#14188.Why is it important/What is the impact to the user?
Permit to the user to automatically clean the DLQ segments once consumed, if the feature is implemented in the target Logstash.
Checklist
[ ] I have commented my code, particularly in hard-to-understand areas[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files (and/or docker env variables)[ ] I have added tests that prove my fix is effective or that my feature worksAuthor's Checklist
How to test this PR locally
Testing is very similar to PR elastic/logstash#14255, it needs an upstream pipeline that produces DLQ events to be stored in DLQ segments, and a downstream pipeline with a DLQ input with
clean_consumed
feature enabled.Verify that tail segments are removed.
DLQ feature needs to be enabled in
logstash.yml
.logstash.yml
enable DLQ feature:clean_consumed
enabled:Related issues
Use cases
As user with a DLQ producer and a consumer pipeline, Logstash as to be able to automatically free the DLQ space used by already consumed events from the consumer side.