Simplify Pipeline class Hierarchy #14551
Conversation
|
Build failed fetching integration resources. Jenkins test this again please |
|
Build will continue to fail until daily snapshots are available for Elasticsearch's newly-cut branch of 8.6, which could be up to 24h. I will follow up tomorrow to kick off CI again. |
| * JRuby extension to provide ancestor class for the ruby-defined {@code LogStash::JavaPipeline} class. | ||
| * | ||
| * <p>NOTE: Although this class' name implies that it is "abstract", we instantiated it directly | ||
| * as a lightweight temporary-scoped pipeline in the ruby-defined {@code LogStash::PipelineAction::Reload} | ||
| * */ | ||
| @JRubyClass(name = "AbstractPipeline") | ||
| public class AbstractPipelineExt extends RubyBasicObject { |
There was a problem hiding this comment.
Thanks for the effort @yaauie !
+1 for removing JavaBasePipelineExt.java and it seems it even doesn't have any test cases.
Out of scope: how do you think for the long term dividing this class into domains:
- DLQ features (with its metrics)
- Stats metrics
- Pipeline related (reloadable, close, etc...)
We probably need to change the inheritance to include java class if we divide (or extend interface if Ruby/JRuby supports).
I would also argue calling this class abstract (it doesn't in a reality). Maybe candidates: PipelineExtension /PipelineDomain/PipelineService.
There was a problem hiding this comment.
I am leaning toward BasePipeline, since it is not abstract. After we have a green CI pass here, I will perform another automated-rename in a separate commit.
There was a problem hiding this comment.
I do not have any specific plans for the cleanup of this class and rearrangement. This PR is one step to remove the hierarchy, and everything else is out of scope.
|
|
||
| public AbstractPipelineExt(final Ruby runtime, final RubyClass metaClass) { | ||
| super(runtime, metaClass); | ||
| } | ||
|
|
||
| @JRubyMethod(required = 4) | ||
| public AbstractPipelineExt initialize(final ThreadContext context, final IRubyObject[] args) |
There was a problem hiding this comment.
Can we have a documentation for this method? I really confuse whenever I do see at this. We have a caller in reload.rb:L51
LogStash::AbstractPipeline.new(@pipeline_config, nil, logger, nil)
If we do not use args[1] & args[3] as they are nil, why don't we remove?
There was a problem hiding this comment.
As stated in the PR, this is a net-zero-change PR, very intentionally designed to change only the hierarchy using automated code tools and as little manual intervention as possible.
I have very little idea what several of these pieces do, and we will have opportunity to refactor and consolidate separately.
|
It looks like we have a Stack 8.6 snapshot now. Jenkins test this again please |
Release notes
[rn: skip]
What does this PR do?
Simplifies the inheritance hierarchy of our
LogStash::JavaPipelineusing nearly-entirely-automated code tools. This is a net-zero-change pull-request.This removes artifacts of the current only-pipeline implementation used by our only-execution engine (previously Java Execution Engine, or JEE) needing to share context with the now-removed
LogStash::Pipelinethat was integral to the deprecated-and-removed Ruby Execution Engine (REE).JavaBasePipelineExtinto its parentAbstractPipelineExtLOGGERand an identicalserialVersionUID, which were manually consolidatedJavaBasePipelineExtLogStash::JavaPipelineinherit directly from our newly-consolidatedAbstractPipelineExtJavaBasePipelineExtto its parentAbstractPipelineExtJavaBasePipelineExt, ensuring no dangling code-references to its ruby formLogStash::JavaBasePipelineWhy is it important/What is the impact to the user?
Simplifies our maintenance footprint, and opens the door for future refactors that can further consolidate or clean up the pipeline's implementation.
Prior to this change, our pipeline code was split between three places:
LogStash::JavaPipelineorg.logstash.execution.JavaBasePipelineExtorg.logstash.execution.AbstractPipelineExtorg.jruby.RubyBasicObjectAfter this change, our pipeline code is one step simpler:
LogStash::JavaPipelineorg.logstash.execution.AbstractPipelineExtorg.jruby.RubyBasicObjectOUT OF SCOPE: the resulting combined
AbstractPipelineExtclass is certainly convoluted, and contains significant duplication, but is effectively identical to the existing as-composedJavaBasePipelineand resolving the complexity and/or duplication is out of scope.Checklist
- [ ] I have made corresponding changes to the documentation- [ ] I have made corresponding change to the default configuration files (and/or docker env variables)How to test this PR locally
As a net-zero-change PR, there is no specific way to validate.