reset refitting_level_ flag to false in error paths #7403
Conversation
ramvadiv
force-pushed
the
fix_refit_level_error_path
branch
from
57d99d3
to
e9fce69
Compare
|
I restarted the failed jobs. This test I've been trying to deflake for a while so will try fixing this failure: This one's surprising. Maybe it is flaky: For testing, I actually did not find much tests for manual compaction failure scenarios (maybe that's why there's so many bugs). Here's one I wrote recently that might be a good starting point: https://github.com/ajkr/rocksdb/blob/46cf49c896dc4a53bee44c5ba807205610125e24/db/db_compaction_test.cc#L5445-L5537. I think the key difference is you want the L0->L1 to be issued before the |
|
This test runs fine in my devserver - but seems to fail pretty consistently on TravisCI. Will investigate more before pushing this thru... |
|
BTW, it's a bit hidden under all the test output, but just want to make sure it's noticed that the last paragraph in my above comment has a suggestion for how to test this bug fix. Admittedly, the suggestion requires the most effort per additional line covered I can imagine -- but we are really lacking in |
|
Sorry for spam. I also forgot again to mention we should add a release note in HISTORY.md under "Unreleased" "Bug fixes" section. |
ramvadiv
force-pushed
the
fix_refit_level_error_path
branch
from
7cef1b7
to
e51ccb8
Compare
|
Andrew - thanks a lot for the pointers/guidance on the test case. I have added a new test to exercise the RefitLevel() error path first and then retest the healthy path. Ran it with old RefitLevel() code and ensured it fails as expected and reran with the fix to ensure it passes. |
ramvadiv
force-pushed
the
fix_refit_level_error_path
branch
3 times, most recently
from
1a6cbff
to
9e65dda
Compare
There was a problem hiding this comment.
@ramvadiv has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
2 similar comments
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@ramvadiv has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
| CompactRangeOptions cro; | ||
| cro.change_level = true; | ||
| cro.target_level = 1; | ||
| ASSERT_NOK(dbfull()->CompactRange(cro, &begin, &end)); |
There was a problem hiding this comment.
Nice, I didn't notice the failure can be triggered without any sync points.
…acky failure in circleci build-linux
…ConflictsWithManual()
…ed in the PR review)
ramvadiv
force-pushed
the
fix_refit_level_error_path
branch
from
1809135
to
e0c65ce
Compare
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@ramvadiv has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
1 similar comment
|
@ramvadiv has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@ramvadiv has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: Reset refitting_level_ flag to false in error paths in DBImpl::ReFitLevel() Pull Request resolved: facebook#7403 Reviewed By: ajkr Differential Revision: D23909028 Pulled By: ramvadiv fbshipit-source-id: 521ad9aadc1b734bef9ef9119d1e1ee1fa8126e9
Reset refitting_level_ flag to false in error paths in DBImpl::ReFitLevel()