Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose the initial logger creation error #10223

Closed
wants to merge 3 commits into from
Closed

Expose the initial logger creation error #10223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c6e14d5
Expose the initial logger creation error
riversand963 Jun 21, 2022
5838d92
Fix ASSERT_STATUS_CHECKED=1
riversand963 Jun 21, 2022
8cec2b3
Fix ASSERT_STATUS_CHECKED=1
riversand963 Jun 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion db/db_basic_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4143,7 +4143,7 @@ TEST_F(DBBasicTest, FailOpenIfLoggerCreationFail) {

Status s = TryReopen(options);
ASSERT_EQ(nullptr, options.info_log);
ASSERT_TRUE(s.IsAborted());
ASSERT_TRUE(s.IsIOError());

SyncPoint::GetInstance()->DisableProcessing();
SyncPoint::GetInstance()->ClearAllCallBacks();
Expand Down
7 changes: 6 additions & 1 deletion db/db_impl/db_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,9 @@ DBImpl::DBImpl(const DBOptions& options, const std::string& dbname,
bool read_only)
: dbname_(dbname),
own_info_log_(options.info_log == nullptr),
initial_db_options_(SanitizeOptions(dbname, options, read_only)),
init_logger_creation_s_(),
initial_db_options_(SanitizeOptions(dbname, options, read_only,
&init_logger_creation_s_)),
env_(initial_db_options_.env),
io_tracer_(std::make_shared<IOTracer>()),
immutable_db_options_(initial_db_options_),
Expand Down Expand Up @@ -747,6 +749,9 @@ Status DBImpl::CloseHelper() {
Status DBImpl::CloseImpl() { return CloseHelper(); }

DBImpl::~DBImpl() {
// TODO: remove this.
init_logger_creation_s_.PermitUncheckedError();

InstrumentedMutexLock closing_lock_guard(&closing_mutex_);
if (closed_) {
return;
Expand Down
7 changes: 5 additions & 2 deletions db/db_impl/db_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1248,6 +1248,7 @@ class DBImpl : public DB {
std::unique_ptr<VersionSet> versions_;
// Flag to check whether we allocated and own the info log file
bool own_info_log_;
Status init_logger_creation_s_;
const DBOptions initial_db_options_;
Env* const env_;
std::shared_ptr<IOTracer> io_tracer_;
Expand Down Expand Up @@ -2600,10 +2601,12 @@ class GetWithTimestampReadCallback : public ReadCallback {
};

extern Options SanitizeOptions(const std::string& db, const Options& src,
bool read_only = false);
bool read_only = false,
Status* logger_creation_s = nullptr);

extern DBOptions SanitizeOptions(const std::string& db, const DBOptions& src,
bool read_only = false);
bool read_only = false,
Status* logger_creation_s = nullptr);

extern CompressionType GetCompressionFlush(
const ImmutableCFOptions& ioptions,
Expand Down
14 changes: 10 additions & 4 deletions db/db_impl/db_impl_open.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,17 @@

namespace ROCKSDB_NAMESPACE {
Options SanitizeOptions(const std::string& dbname, const Options& src,
bool read_only) {
auto db_options = SanitizeOptions(dbname, DBOptions(src), read_only);
bool read_only, Status* logger_creation_s) {
auto db_options =
SanitizeOptions(dbname, DBOptions(src), read_only, logger_creation_s);
ImmutableDBOptions immutable_db_options(db_options);
auto cf_options =
SanitizeOptions(immutable_db_options, ColumnFamilyOptions(src));
return Options(db_options, cf_options);
}

DBOptions SanitizeOptions(const std::string& dbname, const DBOptions& src,
bool read_only) {
bool read_only, Status* logger_creation_s) {
DBOptions result(src);

if (result.env == nullptr) {
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have ASSERT_STATUS_CHECKED failures; if the simplest solution is to PermitUncheckedError() here, that's OK with me (not ideal)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep this fix simple, updated the PR to take the first approach.

}
}
}

Expand Down Expand Up @@ -1790,9 +1794,11 @@ Status DBImpl::Open(const DBOptions& db_options, const std::string& dbname,

DBImpl* impl = new DBImpl(db_options, dbname, seq_per_batch, batch_per_txn);
if (!impl->immutable_db_options_.info_log) {
s = Status::Aborted("Failed to create logger");
s = impl->init_logger_creation_s_;
delete impl;
return s;
} else {
assert(impl->init_logger_creation_s_.ok());
}
s = impl->env_->CreateDirIfMissing(impl->immutable_db_options_.GetWalDir());
if (s.ok()) {
Expand Down
2 changes: 1 addition & 1 deletion utilities/backup/backup_engine_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3066,7 +3066,7 @@ TEST_F(BackupEngineTest, OpenBackupAsReadOnlyDB) {
db = nullptr;

// Now try opening read-write and make sure it fails, for safety.
ASSERT_TRUE(DB::Open(opts, name, &db).IsAborted());
ASSERT_TRUE(DB::Open(opts, name, &db).IsIOError());
}

TEST_F(BackupEngineTest, ProgressCallbackDuringBackup) {
Expand Down