Expose the initial logger creation error #10223
Conversation
|
@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| @@ -59,6 +60,9 @@ DBOptions SanitizeOptions(const std::string& dbname, const DBOptions& src, | |||
| if (!s.ok()) { | |||
| // No place suitable for logging | |||
| result.info_log = nullptr; | |||
| if (logger_creation_s) { | |||
| *logger_creation_s = s; | |||
There was a problem hiding this comment.
We have ASSERT_STATUS_CHECKED failures; if the simplest solution is to PermitUncheckedError() here, that's OK with me (not ideal)
There was a problem hiding this comment.
A single PermitUncheckedError() here does not fully fix the issue. We can either:
- Put a PermitUncheckedError() in ~DBImpl(), or
- For each of CompactedDB, DBImplSecondary, DBImplReadOnly, explicitly call
PermitUncheckedError()shortly after we are done with logger creation.
The latest commit goes by the second approach.
There was a problem hiding this comment.
To keep this fix simple, updated the PR to take the first approach.
riversand963
force-pushed
the
expose-error-CreateLogger
branch
from
a85f4b7 to
5838d92
Compare
|
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
|
@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
|
@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: facebook#9984 changes the behavior of RocksDB: if logger creation failed during `SanitizeOptions()`, `DB::Open()` will fail. However, since `SanitizeOptions()` is called in `DBImpl::DBImpl()`, we cannot directly expose the error to caller without some additional work. This is a first version proposal which: - Adds a new member `init_logger_creation_s` to `DBImpl` to store the result of init logger creation - Checks the error during `DB::Open()` and return it to caller if non-ok This is not very ideal. We can alternatively move the logger creation logic out of the `SanitizeOptions()`. Since `SanitizeOptions()` is used in other places, we need to check whether this change breaks anything in case other callers of `SanitizeOptions()` assumes that a logger should be created. Pull Request resolved: facebook#10223 Test Plan: make check Reviewed By: pdillinger Differential Revision: D37321717 Pulled By: riversand963 fbshipit-source-id: 58042358a86369d606549dd9938933dd47591c4b
#9984 changes the behavior of RocksDB: if logger creation failed during
SanitizeOptions(),DB::Open()will fail. However, sinceSanitizeOptions()is called inDBImpl::DBImpl(), we cannotdirectly expose the error to caller without some additional work.
This is a first version proposal which:
init_logger_creation_stoDBImplto store the result of init logger creationDB::Open()and return it to caller if non-okThis is not very ideal. We can alternatively move the logger creation logic out of the
SanitizeOptions().Since
SanitizeOptions()is used in other places, we need to check whether this change breaks anythingin case other callers of
SanitizeOptions()assumes that a logger should be created.Test plan
make check