[AUTOCUT] Gradle Check Flaky Test Report for TDigestStateTests

[opensearch-ci-bot]

Flaky Test Report for TDigestStateTests

Noticed the TDigestStateTests has some flaky, failing tests that failed during post-merge actions.

Details

Git Reference	Merged Pull Request	Build Details	Test Name
69bf501	18504	59396	org.opensearch.search.aggregations.metrics.TDigestStateTests.testMoreThan4BValues
bd2119f	18468	58962	org.opensearch.search.aggregations.metrics.TDigestStateTests.testMoreThan4BValues

The other pull requests, besides those involved in post-merge actions, that contain failing tests with the TDigestStateTests class are:

• 18070
• 18414
• 18421
• 18439
• 18479
• 18483
• 18498
• 18538

For more details on the failed tests refer to OpenSearch Gradle Check Metrics dashboard.

[andrross]

This was likely introduced by #18124

@peteralfonsi Can you take a look?

/cc @msfroh

[andrross]

This is reproducible with the following seed:

REPRODUCE WITH: ./gradlew ':server:test' --tests "org.opensearch.search.aggregations.metrics.TDigestStateTests.testMoreThan4BValues" -Dtests.seed=A4C86660C6C07D13 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=bem-ZM -Dtests.timezone=Europe/Podgorica -Druntime.java=21

TDigestStateTests > testMoreThan4BValues FAILED
    java.lang.AssertionError
        at __randomizedtesting.SeedInfo.seed([A4C86660C6C07D13:A6D8F53031309131]:0)
        at com.tdunning.math.stats.MergingDigest.merge(MergingDigest.java:490)
        at com.tdunning.math.stats.MergingDigest.mergeNewValues(MergingDigest.java:363)
        at com.tdunning.math.stats.MergingDigest.mergeNewValues(MergingDigest.java:353)
        at com.tdunning.math.stats.MergingDigest.quantile(MergingDigest.java:702)
        at org.opensearch.search.aggregations.metrics.TDigestStateTests.testMoreThan4BValues(TDigestStateTests.java:58)

[msfroh]

@peteralfonsi -- it looks like switching to MergingDigest was considered twice for Elasticsearch before the fork (old issues, should be safe to view):
elastic/elasticsearch#28702 and elastic/elasticsearch#35182.

Given that both of those attempts failed, I think it's probably more effort than we initially considered. This flaky test makes it sound like something is going wrong in the merging logic.

[peteralfonsi]

Thanks for tagging me in this. I think this is probably just due to this test case using the weighted add() method on line 50. This breaks MergingDigest, see the library author's comment on an issue about the same AssertionError here. Instead we're meant to call normal add() iteratively. (Not sure why they left the method around in MergingDigest if you aren't meant to use it...)

As part of #18124 I had removed usages of weighted add() in favor of calling non-weighted add() iteratively, but I must have missed this one, maybe since it was in test code.

Looking at those ES issues it seems like they were waiting on fixes for different issues that were resolved in version 3.3 of the library (which we use now), and later went ahead and forked the library themselves anyway which is why they abandoned the change.

I will look into this more this week to see if there's something else going on. If we change the test case code to remove weighted add(), I expect this flakiness should disappear. Let me know what you guys think.

[peteralfonsi]

Ah, I missed that the weight is 1 << 29, so the "intended" way would then be to call add() 1 << 29 times...

Worth noting this test only exists because they wanted to catch regressions on tdunning/t-digest#70, which was caused by int overflow in AVLTreeDigest. That int array doesn't exist in MergingDigest. It's possible we might not need the test at all when we use MergingDigest, but this would definitely require some more investigation.

[msfroh]

I'm concerned that the MergingDigest may have other lurking bugs. Given that we're hitting an assert within MergingDigest, it sounds like this test is triggering something that shouldn't happen.

I would suggest that we go back to the old digest for now (which at least makes sure that we don't make things worse), and spend some more time either understanding what's going on inside t-digest or looking into other possible implementations. It does look like t-digest development has stalled for the past couple of years.

[peteralfonsi]

Yeah, that seems like a reasonable approach. I think this particular bug is okay, but who knows about other ones... Let's revert #18124 for now. Shame since it is a lot slower.

If I have time I'll look at the Apache MergingDigest implementation like you mentioned earlier - I guess that one is probably more widely used so it'd hopefully have fewer bugs in it.

I think I'll reopen tdunning/t-digest#167 and see if we can get confirmation from the library author that this bug is solely due to using weighted add(), and if there are any good workarounds for that.