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

RollupIT disables all thread leak detection #31258

Closed
jasontedor opened this issue Jun 12, 2018 · 3 comments
Closed

RollupIT disables all thread leak detection #31258

jasontedor opened this issue Jun 12, 2018 · 3 comments
Assignees
Labels
:StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data

Comments

@jasontedor
Copy link
Member

jasontedor commented Jun 12, 2018

Currently the main rollup integration test disables all thread leak detection:

@ThreadLeakScope(ThreadLeakScope.Scope.NONE)
public class RollupIT extends ESIntegTestCase {

I think that this should be unnecessary, I think that the only thread that should be filtered out here is the ObjectCleanerThread from Netty (relates #31232). It looks like this was added for the trigger_engine_scheduler thread. Can we look into this and see if the thread leak none can be removed and reduced to filter the ObjectCleanerThread only (extend XPackIntegTestCase)?

@jasontedor jasontedor added the :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data label Jun 12, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@jasontedor
Copy link
Member Author

We have reverted the Netty upgrade (#31282, netty/netty#8017) so now I think we should find a way to not need thread leak control here at all.

@polyfractal polyfractal self-assigned this Jun 13, 2018
@polyfractal
Copy link
Contributor

I'm inclined to just delete all of RollupIT.

The actual utility of these tests have degraded over time to account for variance... they were supposed to test a end-to-end run and check for accuracy. But this is hard to check algorithmically, especially once doubles get involved. So it's mostly just verifying doc counts now, which we are doing in unit tests elsewhere.

It is also flaky, with some transient failures (#30232). I'm fairly certain are related to tests running over physical midnight -- which changes doc counts slightly -- but I haven't been able to reproduce yet.

It feels like chasing a flaky IT isn't the best use of time (I'm sure @rjernst won't mind deleting an IT :P ). testBig is the only one of semi-value... the other tests have since been replicated in yaml tests.

@colings86 do you have any thoughts? Mind if I just remove this whole test suite? If we see value in this sort of test, I'm still inclined to delete it and redo it better as a QA test.

polyfractal added a commit to polyfractal/elasticsearch that referenced this issue Jul 11, 2018
The old RollupIT was a node IT, an flaky for a number of reasons.
This new version is an ESRestTestCase and should be a little more robust.

This was added to the multi-node QA tests as that seemed like the most
appropriate location.  It didn't seem necessary to create a whole new
QA module.

Note: The only test that was ported was the "Big" test for validating
a larger dataset.  The rest of the tests are represented in existing
yaml tests.

Closes elastic#31258
Closes elastic#30232
Related to elastic#30290
polyfractal added a commit that referenced this issue Jul 16, 2018
The old RollupIT was a node IT, an flaky for a number of reasons.
This new version is an ESRestTestCase and should be a little more robust.

This was added to the multi-node QA tests as that seemed like the most
appropriate location.  It didn't seem necessary to create a whole new
QA module.

Note: The only test that was ported was the "Big" test for validating
a larger dataset.  The rest of the tests are represented in existing
yaml tests.

Closes #31258
Closes #30232
Related to #30290
polyfractal added a commit that referenced this issue Jul 16, 2018
The old RollupIT was a node IT, an flaky for a number of reasons.
This new version is an ESRestTestCase and should be a little more robust.

This was added to the multi-node QA tests as that seemed like the most
appropriate location.  It didn't seem necessary to create a whole new
QA module.

Note: The only test that was ported was the "Big" test for validating
a larger dataset.  The rest of the tests are represented in existing
yaml tests.

Closes #31258
Closes #30232
Related to #30290
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data
Projects
None yet
Development

No branches or pull requests

3 participants