INGEST: Move all Pipeline State into IngestService #32617
Conversation
original-brownbear
commented
Aug 3, 2018
original-brownbear
commented
- Moves all pipeline state into the ingest service
- Retains the existing pipeline store and pipeline execution service as inner classes to make the review easier, they should be flattened out in the next step
- All tests for these classes were copied (and adapted) to the ingest service tests
- This is a refactoring step to enable a clean implementation of a pipeline processor (See INGEST: Add Pipeline Processor #32473)
* Moves all pipeline state into the ingest service * Retains the existing pipeline store and pipeline execution service as inner classes to make the review easier, they should be flattened out in the next step * All tests for these classes were copied (and adapted) to the ingest service tests * This is a refactoring step to enable a clean implementation of a pipeline processor (See elastic#32473)
original-brownbear
added
WIP
:Data Management/Ingest Node
|
Pinging @elastic/es-core-infra |
There was a problem hiding this comment.
Do we really need to keep classes for PipelineExecutionService and PipelineStore? What I meant by my original suggestion was to actually fold the implementation/methods of these classes into IngestService.
Also, /cc @martijnvg who should review as he is more familiar with why these were built as separate services in the first place.
|
@rjernst <= 🎂 on to business: See my PR description, the only reason I did it this way was to make it easier to review. I was gonna fold those together for sure in the next step, the classes just introduce a bunch of complication without benefit outside of making the review easier here imo. |
|
@martijnvg see #32473 (comment) for the motivation behind this. |
|
Jenkins test this |
|
@martijnvg can you take a look here please? :) |
There was a problem hiding this comment.
@original-brownbear LGTM. I just got back from vacation :)
There was no particular reason why these services were separated classes, encapsulating PipelineExecutionService and PipelineStore into IngestService make sense to me.
|
@martijnvg right (with the vacation thing), sorry for being annoying and thanks for the review! :) Merging then, will further flatten this out like suggested by @rjernst in a follow up :) |
* Follow up to elastic#32617 * Flatten redundant inner classes of `IngestService`
* INGEST: Simplify IngestService * Follow up to #32617 * Flatten redundant inner classes of `IngestService`
* INGEST: Simplify IngestService * Follow up to elastic#32617 * Flatten redundant inner classes of `IngestService`
5 participants
