-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Revert "Switch percentiles implementation to MergingDigest (#18124)" #18497
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
Revert "Switch percentiles implementation to MergingDigest (#18124)" #18497
Conversation
…h-project#18124)" This reverts commit 22a6194. Signed-off-by: Peter Alfonsi <[email protected]>
dd10f51
to
13c2261
Compare
❌ Gradle check result for 13c2261: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Flaky test: #14309 |
Signed-off-by: Peter Alfonsi <[email protected]>
Signed-off-by: Peter Alfonsi <[email protected]>
❌ Gradle check result for db4a6c1: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Flaky test: #14324 |
Signed-off-by: Peter Alfonsi <[email protected]>
❌ Gradle check result for 0ebe747: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Peter Alfonsi <[email protected]>
❌ Gradle check result for ac009e8: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Flaky test: #17154 |
Signed-off-by: Peter Alfonsi <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #18497 +/- ##
============================================
- Coverage 72.71% 72.69% -0.02%
- Complexity 68094 68127 +33
============================================
Files 5537 5537
Lines 313312 313352 +40
Branches 45460 45469 +9
============================================
- Hits 227813 227784 -29
- Misses 66967 67082 +115
+ Partials 18532 18486 -46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Thanks @peteralfonsi! |
@msfroh @peteralfonsi Let's make sure this revert gets into the 3.1 branch so that we revert this from the 3.1 release |
…18497) --------- Signed-off-by: Peter Alfonsi <[email protected]> Co-authored-by: Peter Alfonsi <[email protected]> (cherry picked from commit afb08a0) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…h-project#18124)" (opensearch-project#18497) --------- Signed-off-by: Peter Alfonsi <[email protected]> Co-authored-by: Peter Alfonsi <[email protected]>
…h-project#18124)" (opensearch-project#18497) --------- Signed-off-by: Peter Alfonsi <[email protected]> Co-authored-by: Peter Alfonsi <[email protected]>
…18497) --------- Signed-off-by: Peter Alfonsi <[email protected]> Co-authored-by: Peter Alfonsi <[email protected]> (cherry picked from commit afb08a0) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…18497) (#18517) --------- (cherry picked from commit afb08a0) Signed-off-by: Peter Alfonsi <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Peter Alfonsi <[email protected]>
…h-project#18124)" (opensearch-project#18497) --------- Signed-off-by: Peter Alfonsi <[email protected]> Co-authored-by: Peter Alfonsi <[email protected]>Signed-off-by: TJ Neuenfeldt <[email protected]>
…h-project#18124)" (opensearch-project#18497) --------- Signed-off-by: Peter Alfonsi <[email protected]> Co-authored-by: Peter Alfonsi <[email protected]>
Description
Reverts #18124.
As discussed with @msfroh in #18476, we are seeing some worrying AssertionErrors coming out of MergingDigest.quantile() in flaky tests. While that assertion error in particular is probably just due to tdunning/t-digest#167 and doesn't suggest broader issues with correctness, we're concerned there might be other lurking bugs in the MergingDigest implementation. So, we are reverting to the older AVLTreeDigest. Since there haven't been any major version releases since the original PR was merged, there shouldn't be any BWC issues.
I'll also reach out to the library creator and see if we can get some more info on this issue. If there's time I'll investigate the Apache implementation of MergingDigest as well.
Related Issues
Resolves #18476
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.