-
Notifications
You must be signed in to change notification settings - Fork 3.5k
PQ settings refactor: propagate builder upward #18180
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
The `ackedqueue.SettingsImpl` uses an _immutable_ builder, which makes adding options cumbersome; each additional property added needs to modify code from all existing options. By introducing an api-internal temporary mutable builder, we can simplify the process of creating an immutable copy that has a single component modified.
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
This pull request does not have a backport label. Could you fix it @yaauie? 🙏
|
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.
The changes seems fine, I left a question regarding the need to use a MutableBuilder
bridge class to collect field values.
LMK your thoughts about that.
logstash-core/src/main/java/org/logstash/ackedqueue/SettingsImpl.java
Outdated
Show resolved
Hide resolved
logstash-core/src/main/java/org/logstash/ackedqueue/ext/JRubyWrappedAckedQueueExt.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Andrea Selva <[email protected]>
|
💚 Build Succeeded
History
|
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
[rn: skip]
What does this PR do?
This is an internal no-net-change refactor to pull upward the use of the
org.logstash.ackedqueue.Settings
into the JRuby queue extension classes, pared off of the existing PR #18121.Why is it important/What is the impact to the user?
There is no net change to the user.
The changes here underly code that is also going to need changes in @andsel 's current in-flight work. Paring it off as a separate PR unblocks his work.
Checklist
[ ] 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 worksHow to test this PR locally
No net change. Rely on existing test coverage and usage.
Related issues
Use cases
Screenshots
Logs