Remove the return value of SetBGError()
#12792
Conversation
d1aa0b1
to
625a36a
Compare
SetBGError()
|
@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| @@ -851,13 +851,17 @@ TEST_F(DBErrorHandlingFSTest, DoubleManifestWriteError) { | |||
| }); | |||
| SyncPoint::GetInstance()->EnableProcessing(); | |||
| s = Flush(); | |||
| ASSERT_EQ(s.severity(), ROCKSDB_NAMESPACE::Status::Severity::kHardError); | |||
| ASSERT_TRUE(s.IsNoSpace()); | |||
| ASSERT_EQ(dbfull()->TEST_GetBGError().severity(), | |||
There was a problem hiding this comment.
For error severity, the only documentation that I found is "If the database instance goes into read-only mode, the following foreground operations will return the error status on all subsequent calls -" from https://github.com/facebook/rocksdb/wiki/Background-Error-Handling#detection which suggests errors returned after DB turns into read-only mode should have the severity set. In that case, I believe we return bg_error_ from error_handler which will have the severity.
|
@cbi42 has updated the pull request. You must reimport the pull request before landing. |
|
@cbi42 has updated the pull request. You must reimport the pull request before landing. |
|
@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: the return value for
ErrorHandler::SetBGError(error)seems to be not well-defined, it can bebg_error_(no matter if thebg_error_is set to the input error), ok status orrecovery_error_fromStartRecoverFromRetryableBGIOError(). Therecovery_error_returned may be an OK status.We have only a few places that use the return value of
SetBGError()and they don't need to do so. Using the return value may even be wrong for example inrocksdb/db/db_impl/db_impl_write.cc
Line 2365 in 3ee4d5a
scould be overwritten to OK. This PR changes SetBGError() to return void and clean up relevant code.Test plan: existing unit tests and go over all places where return value of
SetBGError()is used.