Deflake tests of compaction based on compensated file size#8036
Conversation
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@ajkr has updated the pull request. You must reimport the pull request before landing. |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
riversand963
left a comment
riversand963
left a comment
There was a problem hiding this comment.
Thanks @ajkr for investigating the flaky test!
There was a problem hiding this comment.
This test itself is testing something not obvious, and as this PR shows, can become flaky when (not necessarily other) developers make changes elsewhere in the codebase. I would argue that a unit test should be accurate and strict: it either succeeds if no invariant changes, or breaks immediately otherwise. Trying to assert on something which may or may not break if some change happens sounds scary to me.
Nevertheless, kudos to these analysis.
There was a problem hiding this comment.
Yeah, these are not great tests. I'll probably land this since having documented/flaky limits is at least better than having undocumented/flaky limits. Hopefully we still later think of a simpler way to test whether compaction picking takes into account tombstones.
CompactionDeletionTriggerReopen was observed to be flaky recently: https://app.circleci.com/pipelines/github/facebook/rocksdb/6030/workflows/787af4f3-b9f7-4645-8e8d-1fb0ebf05539/jobs/101451. I went through it and the related tests and arrived at different conclusions on what constraints we can expect on DB size. Some constraints got looser and some got tighter. The particular constraint that flaked got a lot looser so at least it would have been prevented.
ajkr
left a comment
ajkr
left a comment
There was a problem hiding this comment.
Thanks for the review!
There was a problem hiding this comment.
Yeah, these are not great tests. I'll probably land this since having documented/flaky limits is at least better than having undocumented/flaky limits. Hopefully we still later think of a simpler way to test whether compaction picking takes into account tombstones.
ajkr
force-pushed
the
deflake-compensated-file-size-tests
branch
from
018eb88 to
2d15f7a
Compare
|
@ajkr has updated the pull request. You must reimport the pull request before landing. |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
3 participants
CompactionDeletionTriggerReopen was observed to be flaky recently:
https://app.circleci.com/pipelines/github/facebook/rocksdb/6030/workflows/787af4f3-b9f7-4645-8e8d-1fb0ebf05539/jobs/101451.
I went through it and the related tests and arrived at different
conclusions on what constraints we can expect on DB size. Some
constraints got looser and some got tighter. The particular constraint
that flaked got a lot looser so at least the flake linked above would have been prevented.