INGEST: Simplify IngestService #33008
Conversation
* Follow up to elastic#32617 * Flatten redundant inner classes of `IngestService`
original-brownbear
added
:Data Management/Ingest Node
|
Pinging @elastic/es-core-infra |
martijnvg
left a comment
martijnvg
left a comment
There was a problem hiding this comment.
I left a couple of comments, LGTM otherwise assuming pr builds pass.
| private final ThreadPool threadPool; | ||
|
|
||
| private final StatsHolder totalStats = new StatsHolder(); | ||
|
|
There was a problem hiding this comment.
nit: remove these white lines?
There was a problem hiding this comment.
yea, that would be nicer => cleaning it up :)
|
|
||
| private final PipelineStore store; | ||
| private final ThreadPool threadPool; | ||
| public void executeBulkRequest(Iterable<DocWriteRequest<?>> actionRequests, |
There was a problem hiding this comment.
I don't think this method need to remain public?
There was a problem hiding this comment.
@martijnvg it does, it's called from org.elasticsearch.action.bulk.TransportBulkAction#processBulkIndexIngestRequest.
| Pipeline pipeline = store.get(pipelineId); | ||
| if (pipeline == null) { | ||
| throw new IllegalArgumentException("pipeline with id [" + pipelineId + "] does not exist"); | ||
| private void innerExecute(IndexRequest indexRequest, Pipeline pipeline) throws Exception { |
There was a problem hiding this comment.
maybe a more descriptive method name?
There was a problem hiding this comment.
@martijnvg any suggestions? :)
I agree that these names are kind of meh, but note: We have a bunch of other inner* methods in this class. In the end they're all kind of redundant since they're only used in one place (at least in production code that is) each => somewhat hard to find a better name for a method that effectively only exists to shorten the code of whatever method name that comes after the inner prefix.
There was a problem hiding this comment.
The inner method prefix was introduced to make the actual logic easier to unit test.
I was more referring to the execute part of this method, which is very generic :)
maybe innerIngest()?
There was a problem hiding this comment.
@martijnvg I like that (innerIngest), renaming :)
|
@martijnvg thanks for the review! |
* INGEST: Simplify IngestService * Follow up to elastic#32617 * Flatten redundant inner classes of `IngestService`
4 participants
IngestService