Skip to content
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

Fix TimeSeriesRateAggregatorTests file leak #115278

Merged
merged 3 commits into from
Oct 25, 2024
Merged

Conversation

cbuescher
Copy link
Member

@cbuescher cbuescher commented Oct 21, 2024

With Lucene 10, IndexWriter requires a parent document field in order to use index sorting with document blocks. In production code, we already use "setParentField" when creating an index writer configuration in InternalEngine, but some aggregation tests with nested docs still seem to use index writers without the parent field set. The IAE exception we get here now that Lucene 10 is merged hides another IAE that was tested for in TimeSeriesRateAggregatorTests#testNestedWithinAutoDateHistogram and on top of that occasionally leads to test failures due to file leaks as observed in #115238.

Closes #115238

@cbuescher cbuescher added >test Issues or PRs that are addressing/adding tests :Analytics/Aggregations Aggregations v9.0.0 labels Oct 21, 2024
@cbuescher cbuescher requested a review from kkrik-es October 21, 2024 20:26
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Oct 21, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@cbuescher
Copy link
Member Author

@kkrik-es you seem to be familiar with the Lucene changes in this area, is my reasoning above correct?
The IAE we get without this change now satisfies expected exception class in the test in question but since its thrown earlier can lead to the IndexWriter not being closed properly, which occasionally gets detected by the test infra as a file leak.

@cbuescher
Copy link
Member Author

@kkrik-es I saw you fixed the Leak issue with the change in #115345, however I believe that we are now throwing a different IAE than before the merge of Lucene 10 (in a different location), so I think this change here still applies. When the parent field is set we don't need to add documents separately and get back the "old" IAE from before the Lucene 10 update. Would you mind taking a look and let me know if my thinking here is right?

@cbuescher
Copy link
Member Author

/cc @iverase

@kkrik-es
Copy link
Contributor

I am really not familiar with these changes, just noticed that @iverase applied them elsewhere..

If Ignacio is fine with it and the following test passes, I've no objection:

./gradlew ":x-pack:plugin:analytics:test" --tests "org.elasticsearch.xpack.analytics.rate.TimeSeriesRateAggregatorTests" -Dtests.seed=509E2AFA17E6D235 -Dtests.locale=en -Dtests.timezone=Etc/UTC -Druntime.java=22

@cbuescher
Copy link
Member Author

./gradlew ":x-pack:plugin:analytics:test" --tests "org.elasticsearch.xpack.analytics.rate.TimeSeriesRateAggregatorTests" -Dtests.seed=509E2AFA17E6D235 -Dtests.locale=en -Dtests.timezone=Etc/UTC -Druntime.java=22

That passes. I asked you because this change in InternalEngine that also sets the parent field was made by you. I think the change in the IAE that is being thrown went unnoticed because we don't check it message so far (which I added here)

@cbuescher cbuescher requested a review from iverase October 25, 2024 10:52
@kkrik-es
Copy link
Contributor

I asked you because this change in InternalEngine that also sets the parent field was made by you. I think the change in the IAE that is being thrown went unnoticed because we don't check it message so far (which I added here)

I see, so we were missing setting the parent doc so there was a mismatch in Lucene land that led to errors? At any rate, I've no objection here.

@cbuescher
Copy link
Member Author

@elasticmachine elasticsearch-ci/part-2

@cbuescher cbuescher merged commit 6cec96c into elastic:main Oct 25, 2024
16 checks passed
jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request Nov 4, 2024
With Lucene 10, IndexWriter requires a parent document field in order to
use index sorting with document blocks. This lead to different IAE and file
leaks in this test which are fixed by adapting the corresponding location in
the test setup.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] TimeSeriesRateAggregatorTests fails with file handle leak
3 participants