Skip to content

Fix test failure with double operation precision#88803

Merged
kingherc merged 1 commit intoelastic:mainfrom
kingherc:test-failure/88791-minimum-low-watermark
Jul 26, 2022
Merged

Fix test failure with double operation precision#88803
kingherc merged 1 commit intoelastic:mainfrom
kingherc:test-failure/88791-minimum-low-watermark

Conversation

@kingherc
Copy link
Copy Markdown
Contributor

@kingherc kingherc commented Jul 26, 2022

Fixes #88791

@kingherc kingherc requested a review from mark-vieira July 26, 2022 10:42
@kingherc kingherc added :Core/Infra/Settings Settings infrastructure and APIs >test-failure Triaged test failures from CI labels Jul 26, 2022
@kingherc kingherc self-assigned this Jul 26, 2022
@kingherc kingherc marked this pull request as ready for review July 26, 2022 10:42
@kingherc kingherc removed the request for review from mark-vieira July 26, 2022 10:43
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jul 26, 2022
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

elasticsearchmachine commented Jul 26, 2022

Pinging @elastic/es-core-infra (Team:Core/Infra)

@kingherc kingherc requested a review from mark-vieira July 26, 2022 10:44
@kingherc kingherc mentioned this pull request Jul 26, 2022
Closed
@kingherc
Copy link
Copy Markdown
Contributor Author

kingherc commented Jul 26, 2022

@mark-vieira I assigned you as reviewer, but feel free to tell me if you would like me to assign someone else. Thanks

@kingherc kingherc requested review from mark-vieira and removed request for mark-vieira July 26, 2022 11:53
@DaveCTurner
Copy link
Copy Markdown
Member

DaveCTurner commented Jul 26, 2022

Did you manage to reproduce the failure? It doesn't reproduce for me, and indeed the test seems pretty solid even after 100k+ iterations.

@kingherc
Copy link
Copy Markdown
Contributor Author

kingherc commented Jul 26, 2022 •
edited
Loading

Did you manage to reproduce the failure? It doesn't reproduce for me, and indeed the test seems pretty solid even after 100k+ iterations.

Yes it reproduces for me. @DaveCTurner Did you remove the @AwaitsFix flag?

Feel free to review this if you'd like. It relates to double rounding errors like the one we saw in #88683 .

In the original function I adapted (see https://github.com/elastic/elasticsearch/pull/88719/files#diff-4295fc3ce1dcb006cead63e125db2910d0c53cecf7ed2e184e4dc07bd569385a ), @henningandersen used the percentage instead of ratio, for possibly the exact same reason. That's why I thought of using the same and indeed it fixed the rounding issue here.

@kingherc kingherc requested a review from DaveCTurner July 26, 2022 12:10
DaveCTurner
DaveCTurner approved these changes Jul 26, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Did you remove the @AwaitsFix flag?

🤦

Got it now. LGTM

@kingherc kingherc merged commit 631a705 into elastic:main Jul 26, 2022
@kingherc kingherc deleted the test-failure/88791-minimum-low-watermark branch July 26, 2022 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DaveCTurner DaveCTurner DaveCTurner approved these changes

@mark-vieira mark-vieira Awaiting requested review from mark-vieira

Assignees

@kingherc kingherc

Labels

:Core/Infra/Settings Settings infrastructure and APIs Team:Core/Infra Meta label for core/infra team >test-failure Triaged test failures from CI v8.4.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[CI] DiskThresholdSettingsTests testMinimumTotalSizeForBelowLowWatermark failing

3 participants

@kingherc @elasticsearchmachine @DaveCTurner