From 62ad0a9b19f0be4cefa70b6b32876e764b7f3c11 Mon Sep 17 00:00:00 2001 From: Igor Canadi Date: Mon, 5 Jan 2015 13:35:56 -0800 Subject: [PATCH] Deprecating skip_log_error_on_recovery Summary: Since https://reviews.facebook.net/D16119, we ignore partial tailing writes. Because of that, we no longer need skip_log_error_on_recovery. The documentation says "Skip log corruption error on recovery (If client is ok with losing most recent changes)", while the option actually ignores any corruption of the WAL (not only just the most recent changes). This is very dangerous and can lead to DB inconsistencies. This was originally set up to ignore partial tailing writes, which we now do automatically (after D16119). I have digged up old task t2416297 which confirms my findings. Test Plan: There was actually no tests that verified correct behavior of skip_log_error_on_recovery. Reviewers: yhchiang, rven, dhruba, sdong Reviewed By: sdong Subscribers: dhruba, leveldb Differential Revision: https://reviews.facebook.net/D30603 --- HISTORY.md | 3 +++ db/db_impl.cc | 8 ++------ include/rocksdb/options.h | 4 +--- util/options.cc | 2 -- 4 files changed, 6 insertions(+), 11 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 49fc56df8..245f4ec61 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -9,6 +9,9 @@ numbered levels will be placed later in the db_paths vector. * Potentially big performance improvements if you're using RocksDB with lots of column families (100-1000) +### Public API changes +* Deprecated skip_log_error_on_recovery option + ### 3.9.0 (12/8/2014) ### New Features diff --git a/db/db_impl.cc b/db/db_impl.cc index e529db3c7..dfe66eeab 100644 --- a/db/db_impl.cc +++ b/db/db_impl.cc @@ -842,8 +842,7 @@ Status DBImpl::RecoverLogFiles(const std::vector& log_numbers, Env* env; Logger* info_log; const char* fname; - Status* status; // nullptr if db_options_.paranoid_checks==false or - // db_options_.skip_log_error_on_recovery==true + Status* status; // nullptr if db_options_.paranoid_checks==false virtual void Corruption(size_t bytes, const Status& s) { Log(InfoLogLevel::WARN_LEVEL, info_log, "%s%s: dropping %d bytes; %s", @@ -888,10 +887,7 @@ Status DBImpl::RecoverLogFiles(const std::vector& log_numbers, reporter.env = env_; reporter.info_log = db_options_.info_log.get(); reporter.fname = fname.c_str(); - reporter.status = - (db_options_.paranoid_checks && !db_options_.skip_log_error_on_recovery - ? &status - : nullptr); + reporter.status = (db_options_.paranoid_checks) ? &status : nullptr; // We intentially make log::Reader do checksumming even if // paranoid_checks==false so that corruptions cause entire commits // to be skipped instead of propagating bad information (like overly diff --git a/include/rocksdb/options.h b/include/rocksdb/options.h index 298ec6aee..75625abcc 100644 --- a/include/rocksdb/options.h +++ b/include/rocksdb/options.h @@ -850,9 +850,7 @@ struct DBOptions { // Disable child process inherit open files. Default: true bool is_fd_close_on_exec; - // Skip log corruption error on recovery (If client is ok with - // losing most recent changes) - // Default: false + // DEPRECATED -- this options is no longer used bool skip_log_error_on_recovery; // if not zero, dump rocksdb.stats to LOG every stats_dump_period_sec diff --git a/util/options.cc b/util/options.cc index 085df053d..75307f13f 100644 --- a/util/options.cc +++ b/util/options.cc @@ -333,8 +333,6 @@ void DBOptions::Dump(Logger* log) const { allow_mmap_writes); Log(log, " Options.is_fd_close_on_exec: %d", is_fd_close_on_exec); - Log(log, " Options.skip_log_error_on_recovery: %d", - skip_log_error_on_recovery); Log(log, " Options.stats_dump_period_sec: %u", stats_dump_period_sec); Log(log, " Options.advise_random_on_open: %d",