Status check enforcement for error_handler_fs_test #7342
Conversation
zhichao-cao
force-pushed
the
status_repair_handler
branch
from
6ef2ccd
to
5528395
Compare
There was a problem hiding this comment.
@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
zhichao-cao
force-pushed
the
status_repair_handler
branch
from
5528395
to
9f71d1d
Compare
|
@zhichao-cao has updated the pull request. You must reimport the pull request before landing. |
zhichao-cao
force-pushed
the
status_repair_handler
branch
from
9f71d1d
to
2f89439
Compare
|
@zhichao-cao has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| @@ -848,7 +849,8 @@ void DBImpl::WriteStatusCheck(const Status& status) { | |||
| if (immutable_db_options_.paranoid_checks && !status.ok() && | |||
| !status.IsBusy() && !status.IsIncomplete()) { | |||
| mutex_.Lock(); | |||
| error_handler_.SetBGError(status, BackgroundErrorReason::kWriteCallback); | |||
| error_handler_.SetBGError(status, BackgroundErrorReason::kWriteCallback) | |||
There was a problem hiding this comment.
-
There are other PRs changing these same things at the moment so prepare for merges.
-
(Same comment on the other PRs): Why not have this method return a void or create an alternate signature that returns a void, rather than ignoring the result everywhere?
There was a problem hiding this comment.
Actually, in the future stage, we want to use the returned states from SetBGError and make the actions. For example, if the error is a retryable I/O error and satisfy some condition, SetBGError might map it to a hard error. When the compaction or flush logic check the returns, it will take actions such as rewrite the file, or fail the current flush. So we prefer to keep the current return.
zhichao-cao
force-pushed
the
status_repair_handler
branch
from
2f89439
to
dc4d12f
Compare
|
@zhichao-cao has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
zhichao-cao
force-pushed
the
status_repair_handler
branch
from
dc4d12f
to
a1fc000
Compare
|
@zhichao-cao has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
db/db_impl/db_impl.cc
Outdated
| @@ -407,7 +407,7 @@ Status DBImpl::ResumeImpl(DBRecoverContext context) { | |||
| // during previous error handling. | |||
| if (file_deletion_disabled) { | |||
| // Always return ok | |||
| EnableFileDeletions(/*force=*/true); | |||
| EnableFileDeletions(/*force=*/true).PermitUncheckedError(); | |||
There was a problem hiding this comment.
Can we add assert or check status here in case if different status is returned in future, PermitUncheckedError might ignore it?
There was a problem hiding this comment.
I will add an assert here, it always return OK.
zhichao-cao
force-pushed
the
status_repair_handler
branch
from
a1fc000
to
6cd5394
Compare
|
@zhichao-cao has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
zhichao-cao
force-pushed
the
status_repair_handler
branch
from
6cd5394
to
ab7ffba
Compare
|
@zhichao-cao has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
zhichao-cao
force-pushed
the
status_repair_handler
branch
from
ab7ffba
to
43f3689
Compare
|
@zhichao-cao has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
zhichao-cao
force-pushed
the
status_repair_handler
branch
from
43f3689
to
39361f4
Compare
|
@zhichao-cao has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@zhichao-cao merged this pull request in b7062f0. |
Summary: Added status check enforcement for error_test_fs_test Pull Request resolved: facebook#7342 Test Plan: ASSERT_STATUS_CHECKED=1 make -j48 error_test_fs_test Reviewed By: akankshamahajan15 Differential Revision: D23972231 Pulled By: zhichao-cao fbshipit-source-id: fa41bfe440012e0c55f2c9507c1d0104e5e93f84
Summary: Added status check enforcement for error_test_fs_test
Test Plan: ASSERT_STATUS_CHECKED=1 make -j48 error_test_fs_test