Skip to content

Conversation

@joegallo
Copy link
Contributor

Related to #92843

Expands the scenarios under test, leaving the failing test assertion commented out. A PR that fixes the bug will be able to remove the comment.

Note: it's a bit odd to add a failing test like this, but I'm working up to something -- I think it'll be easier for everyone if we add the test this way so that I can drop the comment later.

Push a single document through a default/request and a final pipeline
(of one processor apiece) and check the resulting statistics.
@joegallo joegallo added >test Issues or PRs that are addressing/adding tests :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP Team:Data Management Meta label for data/management team labels Jan 20, 2023
@joegallo joegallo requested a review from martijnvg January 20, 2023 18:36
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

new BytesArray("{\"processors\": [{\"mock\" : {}}]}"),
XContentType.JSON
);
PutPipelineRequest putRequest2 = new PutPipelineRequest(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth including a nested pipeline here? I know you and I have discussed that previously, but I'm not sure whether it actually makes the counters any more complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a time-boxed swing at it, I do think it's worth adding (assuming it doesn't blow up the size or complexity of the test extraordinarily).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added via 0bc58d2. It's a bit janky, because there's a chicken and egg problem of the mock ingest service needing the processor factory, but the factory needing an ingest service.

I'm game to take a swing at golfing it, though, if you'd like to join up for 10-15 minutes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks reasonable enough to me.

@joegallo joegallo removed the request for review from martijnvg January 23, 2023 15:44
@joegallo joegallo requested a review from masseyke January 23, 2023 17:07
@joegallo joegallo merged commit 21c8a56 into elastic:main Jan 23, 2023
@joegallo joegallo deleted the ingest-service-double-counting-bug branch January 23, 2023 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP Team:Data Management Meta label for data/management team >test Issues or PRs that are addressing/adding tests v8.7.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants