From d08a9005b76a7c96d25326ecd98890c6d2b77511 Mon Sep 17 00:00:00 2001 From: sdong Date: Tue, 29 Sep 2020 09:47:33 -0700 Subject: [PATCH] Make db_basic_test pass assert status checked (#7452) Summary: Add db_basic_test status check list. Some of the warnings are suppressed. It is possible that some of them are due to real bugs. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7452 Test Plan: See CI tests pass. Reviewed By: zhichao-cao Differential Revision: D23979764 fbshipit-source-id: 6151570c2a9b931b0fbb3fe939a94b2bd1583cbe --- Makefile | 1 + db/builder.cc | 4 +- db/compacted_db_impl.cc | 19 ++- db/compaction/compaction_job.cc | 10 ++ db/db_basic_test.cc | 156 +++++++++--------- db/db_impl/db_impl_compaction_flush.cc | 66 +++++--- db/db_impl/db_impl_files.cc | 9 +- db/db_impl/db_impl_open.cc | 4 + db/db_impl/db_impl_write.cc | 30 ++-- db/db_iter.h | 9 +- db/db_test_util.cc | 6 +- db/error_handler.cc | 2 +- db/table_cache.cc | 40 +++-- db/version_builder.cc | 5 + db/version_set.cc | 2 + .../block_based/block_based_table_builder.cc | 25 ++- table/block_based/block_based_table_reader.cc | 12 +- table/block_based/index_builder.h | 5 +- table/plain/plain_table_builder.cc | 4 + table/plain/plain_table_key_coding.cc | 1 - table/plain/plain_table_key_coding.h | 7 + table/plain/plain_table_reader.cc | 2 + utilities/fault_injection_env.h | 2 + 23 files changed, 273 insertions(+), 148 deletions(-) diff --git a/Makefile b/Makefile index 2e8497e28..3e787149d 100644 --- a/Makefile +++ b/Makefile @@ -586,6 +586,7 @@ ifdef ASSERT_STATUS_CHECKED coding_test \ crc32c_test \ dbformat_test \ + db_basic_test \ db_options_test \ options_file_test \ defer_test \ diff --git a/db/builder.cc b/db/builder.cc index 551016077..da668d604 100644 --- a/db/builder.cc +++ b/db/builder.cc @@ -131,6 +131,7 @@ Status BuildTable( TEST_SYNC_POINT_CALLBACK("BuildTable:create_file", &use_direct_writes); #endif // !NDEBUG IOStatus io_s = NewWritableFile(fs, fname, &file, file_options); + assert(s.ok()); s = io_s; if (io_status->ok()) { *io_status = io_s; @@ -314,17 +315,18 @@ Status BuildTable( constexpr IODebugContext* dbg = nullptr; Status ignored = fs->DeleteFile(fname, IOOptions(), dbg); + ignored.PermitUncheckedError(); assert(blob_file_additions || blob_file_paths.empty()); if (blob_file_additions) { for (const std::string& blob_file_path : blob_file_paths) { ignored = fs->DeleteFile(blob_file_path, IOOptions(), dbg); + ignored.PermitUncheckedError(); } blob_file_additions->clear(); } - ignored.PermitUncheckedError(); } if (meta->fd.GetFileSize() == 0) { diff --git a/db/compacted_db_impl.cc b/db/compacted_db_impl.cc index dc7e8cc38..c704ae9cc 100644 --- a/db/compacted_db_impl.cc +++ b/db/compacted_db_impl.cc @@ -41,8 +41,11 @@ Status CompactedDBImpl::Get(const ReadOptions& options, ColumnFamilyHandle*, GetContext::kNotFound, key, value, nullptr, nullptr, nullptr, true, nullptr, nullptr); LookupKey lkey(key, kMaxSequenceNumber); - files_.files[FindFile(key)].fd.table_reader->Get(options, lkey.internal_key(), - &get_context, nullptr); + Status s = files_.files[FindFile(key)].fd.table_reader->Get( + options, lkey.internal_key(), &get_context, nullptr); + if (!s.ok() && !s.IsNotFound()) { + return s; + } if (get_context.State() == GetContext::kFound) { return Status::OK(); } @@ -74,10 +77,14 @@ std::vector CompactedDBImpl::MultiGet(const ReadOptions& options, GetContext::kNotFound, keys[idx], &pinnable_val, nullptr, nullptr, nullptr, true, nullptr, nullptr); LookupKey lkey(keys[idx], kMaxSequenceNumber); - r->Get(options, lkey.internal_key(), &get_context, nullptr); - value.assign(pinnable_val.data(), pinnable_val.size()); - if (get_context.State() == GetContext::kFound) { - statuses[idx] = Status::OK(); + Status s = r->Get(options, lkey.internal_key(), &get_context, nullptr); + if (!s.ok() && !s.IsNotFound()) { + statuses[idx] = s; + } else { + value.assign(pinnable_val.data(), pinnable_val.size()); + if (get_context.State() == GetContext::kFound) { + statuses[idx] = Status::OK(); + } } } ++idx; diff --git a/db/compaction/compaction_job.cc b/db/compaction/compaction_job.cc index 283d6e9a0..10610b6f6 100644 --- a/db/compaction/compaction_job.cc +++ b/db/compaction/compaction_job.cc @@ -1102,6 +1102,16 @@ void CompactionJob::ProcessKeyValueCompaction(SubcompactionState* sub_compact) { SetPerfLevel(prev_perf_level); } } +#ifdef ROCKSDB_ASSERT_STATUS_CHECKED + if (!status.ok()) { + if (sub_compact->c_iter) { + sub_compact->c_iter->status().PermitUncheckedError(); + } + if (input) { + input->status().PermitUncheckedError(); + } + } +#endif // ROCKSDB_ASSERT_STATUS_CHECKED sub_compact->c_iter.reset(); input.reset(); diff --git a/db/db_basic_test.cc b/db/db_basic_test.cc index 07c754ffd..bc1027a8f 100644 --- a/db/db_basic_test.cc +++ b/db/db_basic_test.cc @@ -35,9 +35,9 @@ class DBBasicTest : public DBTestBase { TEST_F(DBBasicTest, OpenWhenOpen) { Options options = CurrentOptions(); options.env = env_; - ROCKSDB_NAMESPACE::DB* db2 = nullptr; - ROCKSDB_NAMESPACE::Status s = DB::Open(options, dbname_, &db2); - + DB* db2 = nullptr; + Status s = DB::Open(options, dbname_, &db2); + ASSERT_NOK(s); ASSERT_EQ(Status::Code::kIOError, s.code()); ASSERT_EQ(Status::SubCode::kNone, s.subcode()); ASSERT_TRUE(strstr(s.getState(), "lock ") != nullptr); @@ -49,13 +49,13 @@ TEST_F(DBBasicTest, UniqueSession) { Options options = CurrentOptions(); std::string sid1, sid2, sid3, sid4; - db_->GetDbSessionId(sid1); + ASSERT_OK(db_->GetDbSessionId(sid1)); Reopen(options); - db_->GetDbSessionId(sid2); + ASSERT_OK(db_->GetDbSessionId(sid2)); ASSERT_OK(Put("foo", "v1")); - db_->GetDbSessionId(sid4); + ASSERT_OK(db_->GetDbSessionId(sid4)); Reopen(options); - db_->GetDbSessionId(sid3); + ASSERT_OK(db_->GetDbSessionId(sid3)); ASSERT_NE(sid1, sid2); ASSERT_NE(sid1, sid3); @@ -73,14 +73,14 @@ TEST_F(DBBasicTest, UniqueSession) { #ifndef ROCKSDB_LITE Close(); ASSERT_OK(ReadOnlyReopen(options)); - db_->GetDbSessionId(sid1); + ASSERT_OK(db_->GetDbSessionId(sid1)); // Test uniqueness between readonly open (sid1) and regular open (sid3) ASSERT_NE(sid1, sid3); Close(); ASSERT_OK(ReadOnlyReopen(options)); - db_->GetDbSessionId(sid2); + ASSERT_OK(db_->GetDbSessionId(sid2)); ASSERT_EQ("v1", Get("foo")); - db_->GetDbSessionId(sid3); + ASSERT_OK(db_->GetDbSessionId(sid3)); ASSERT_NE(sid1, sid2); @@ -88,13 +88,13 @@ TEST_F(DBBasicTest, UniqueSession) { #endif // ROCKSDB_LITE CreateAndReopenWithCF({"goku"}, options); - db_->GetDbSessionId(sid1); + ASSERT_OK(db_->GetDbSessionId(sid1)); ASSERT_OK(Put("bar", "e1")); - db_->GetDbSessionId(sid2); + ASSERT_OK(db_->GetDbSessionId(sid2)); ASSERT_EQ("e1", Get("bar")); - db_->GetDbSessionId(sid3); + ASSERT_OK(db_->GetDbSessionId(sid3)); ReopenWithColumnFamilies({"default", "goku"}, options); - db_->GetDbSessionId(sid4); + ASSERT_OK(db_->GetDbSessionId(sid4)); ASSERT_EQ(sid1, sid2); ASSERT_EQ(sid2, sid3); @@ -163,7 +163,7 @@ TEST_F(DBBasicTest, ReadOnlyDBWithWriteDBIdToManifestSet) { assert(options.env == env_); ASSERT_OK(ReadOnlyReopen(options)); std::string db_id1; - db_->GetDbIdentity(db_id1); + ASSERT_OK(db_->GetDbIdentity(db_id1)); ASSERT_EQ("v3", Get("foo")); ASSERT_EQ("v2", Get("bar")); Iterator* iter = db_->NewIterator(ReadOptions()); @@ -186,7 +186,7 @@ TEST_F(DBBasicTest, ReadOnlyDBWithWriteDBIdToManifestSet) { ASSERT_EQ("v2", Get("bar")); ASSERT_TRUE(db_->SyncWAL().IsNotSupported()); std::string db_id2; - db_->GetDbIdentity(db_id2); + ASSERT_OK(db_->GetDbIdentity(db_id2)); ASSERT_EQ(db_id1, db_id2); } @@ -241,7 +241,7 @@ TEST_F(DBBasicTest, CompactedDB) { ASSERT_OK(Put("hhh", DummyString(kFileSize / 2, 'h'))); ASSERT_OK(Put("iii", DummyString(kFileSize / 2, 'i'))); ASSERT_OK(Put("jjj", DummyString(kFileSize / 2, 'j'))); - db_->CompactRange(CompactRangeOptions(), nullptr, nullptr); + ASSERT_OK(db_->CompactRange(CompactRangeOptions(), nullptr, nullptr)); ASSERT_EQ(3, NumTableFilesAtLevel(1)); Close(); @@ -299,8 +299,8 @@ TEST_F(DBBasicTest, LevelLimitReopen) { int i = 0; while (NumTableFilesAtLevel(2, 1) == 0) { ASSERT_OK(Put(1, Key(i++), value)); - dbfull()->TEST_WaitForFlushMemTable(); - dbfull()->TEST_WaitForCompact(); + ASSERT_OK(dbfull()->TEST_WaitForFlushMemTable()); + ASSERT_OK(dbfull()->TEST_WaitForCompact()); } options.num_levels = 1; @@ -354,8 +354,8 @@ TEST_F(DBBasicTest, EmptyFlush) { options.disable_auto_compactions = true; CreateAndReopenWithCF({"pikachu"}, options); - Put(1, "a", Slice()); - SingleDelete(1, "a"); + ASSERT_OK(Put(1, "a", Slice())); + ASSERT_OK(SingleDelete(1, "a")); ASSERT_OK(Flush(1)); ASSERT_EQ("[ ]", AllEntriesFor("a", 1)); @@ -605,16 +605,16 @@ TEST_F(DBBasicTest, Snapshot) { options_override.skip_policy = kSkipNoSnapshot; do { CreateAndReopenWithCF({"pikachu"}, CurrentOptions(options_override)); - Put(0, "foo", "0v1"); - Put(1, "foo", "1v1"); + ASSERT_OK(Put(0, "foo", "0v1")); + ASSERT_OK(Put(1, "foo", "1v1")); const Snapshot* s1 = db_->GetSnapshot(); ASSERT_EQ(1U, GetNumSnapshots()); uint64_t time_snap1 = GetTimeOldestSnapshots(); ASSERT_GT(time_snap1, 0U); ASSERT_EQ(GetSequenceOldestSnapshots(), s1->GetSequenceNumber()); - Put(0, "foo", "0v2"); - Put(1, "foo", "1v2"); + ASSERT_OK(Put(0, "foo", "0v2")); + ASSERT_OK(Put(1, "foo", "1v2")); env_->MockSleepForSeconds(1); @@ -622,8 +622,8 @@ TEST_F(DBBasicTest, Snapshot) { ASSERT_EQ(2U, GetNumSnapshots()); ASSERT_EQ(time_snap1, GetTimeOldestSnapshots()); ASSERT_EQ(GetSequenceOldestSnapshots(), s1->GetSequenceNumber()); - Put(0, "foo", "0v3"); - Put(1, "foo", "1v3"); + ASSERT_OK(Put(0, "foo", "0v3")); + ASSERT_OK(Put(1, "foo", "1v3")); { ManagedSnapshot s3(db_); @@ -631,8 +631,8 @@ TEST_F(DBBasicTest, Snapshot) { ASSERT_EQ(time_snap1, GetTimeOldestSnapshots()); ASSERT_EQ(GetSequenceOldestSnapshots(), s1->GetSequenceNumber()); - Put(0, "foo", "0v4"); - Put(1, "foo", "1v4"); + ASSERT_OK(Put(0, "foo", "0v4")); + ASSERT_OK(Put(1, "foo", "1v4")); ASSERT_EQ("0v1", Get(0, "foo", s1)); ASSERT_EQ("1v1", Get(1, "foo", s1)); ASSERT_EQ("0v2", Get(0, "foo", s2)); @@ -698,14 +698,14 @@ TEST_P(DBBasicMultiConfigs, CompactBetweenSnapshots) { Random rnd(301); FillLevels("a", "z", 1); - Put(1, "foo", "first"); + ASSERT_OK(Put(1, "foo", "first")); const Snapshot* snapshot1 = db_->GetSnapshot(); - Put(1, "foo", "second"); - Put(1, "foo", "third"); - Put(1, "foo", "fourth"); + ASSERT_OK(Put(1, "foo", "second")); + ASSERT_OK(Put(1, "foo", "third")); + ASSERT_OK(Put(1, "foo", "fourth")); const Snapshot* snapshot2 = db_->GetSnapshot(); - Put(1, "foo", "fifth"); - Put(1, "foo", "sixth"); + ASSERT_OK(Put(1, "foo", "fifth")); + ASSERT_OK(Put(1, "foo", "sixth")); // All entries (including duplicates) exist // before any compaction or flush is triggered. @@ -723,7 +723,8 @@ TEST_P(DBBasicMultiConfigs, CompactBetweenSnapshots) { // after we release the snapshot1, only two values left db_->ReleaseSnapshot(snapshot1); FillLevels("a", "z", 1); - dbfull()->CompactRange(CompactRangeOptions(), handles_[1], nullptr, nullptr); + ASSERT_OK(dbfull()->CompactRange(CompactRangeOptions(), handles_[1], nullptr, + nullptr)); // We have only one valid snapshot snapshot2. Since snapshot1 is // not valid anymore, "first" should be removed by a compaction. @@ -734,7 +735,8 @@ TEST_P(DBBasicMultiConfigs, CompactBetweenSnapshots) { // after we release the snapshot2, only one value should be left db_->ReleaseSnapshot(snapshot2); FillLevels("a", "z", 1); - dbfull()->CompactRange(CompactRangeOptions(), handles_[1], nullptr, nullptr); + ASSERT_OK(dbfull()->CompactRange(CompactRangeOptions(), handles_[1], nullptr, + nullptr)); ASSERT_EQ("sixth", Get(1, "foo")); ASSERT_EQ(AllEntriesFor("foo", 1), "[ sixth ]"); } @@ -790,18 +792,18 @@ TEST_F(DBBasicTest, CompactOnFlush) { options.disable_auto_compactions = true; CreateAndReopenWithCF({"pikachu"}, options); - Put(1, "foo", "v1"); + ASSERT_OK(Put(1, "foo", "v1")); ASSERT_OK(Flush(1)); ASSERT_EQ(AllEntriesFor("foo", 1), "[ v1 ]"); // Write two new keys - Put(1, "a", "begin"); - Put(1, "z", "end"); - Flush(1); + ASSERT_OK(Put(1, "a", "begin")); + ASSERT_OK(Put(1, "z", "end")); + ASSERT_OK(Flush(1)); // Case1: Delete followed by a put - Delete(1, "foo"); - Put(1, "foo", "v2"); + ASSERT_OK(Delete(1, "foo")); + ASSERT_OK(Put(1, "foo", "v2")); ASSERT_EQ(AllEntriesFor("foo", 1), "[ v2, DEL, v1 ]"); // After the current memtable is flushed, the DEL should @@ -809,66 +811,66 @@ TEST_F(DBBasicTest, CompactOnFlush) { ASSERT_OK(Flush(1)); ASSERT_EQ(AllEntriesFor("foo", 1), "[ v2, v1 ]"); - dbfull()->CompactRange(CompactRangeOptions(), handles_[1], nullptr, - nullptr); + ASSERT_OK(dbfull()->CompactRange(CompactRangeOptions(), handles_[1], + nullptr, nullptr)); ASSERT_EQ(AllEntriesFor("foo", 1), "[ v2 ]"); // Case 2: Delete followed by another delete - Delete(1, "foo"); - Delete(1, "foo"); + ASSERT_OK(Delete(1, "foo")); + ASSERT_OK(Delete(1, "foo")); ASSERT_EQ(AllEntriesFor("foo", 1), "[ DEL, DEL, v2 ]"); ASSERT_OK(Flush(1)); ASSERT_EQ(AllEntriesFor("foo", 1), "[ DEL, v2 ]"); - dbfull()->CompactRange(CompactRangeOptions(), handles_[1], nullptr, - nullptr); + ASSERT_OK(dbfull()->CompactRange(CompactRangeOptions(), handles_[1], + nullptr, nullptr)); ASSERT_EQ(AllEntriesFor("foo", 1), "[ ]"); // Case 3: Put followed by a delete - Put(1, "foo", "v3"); - Delete(1, "foo"); + ASSERT_OK(Put(1, "foo", "v3")); + ASSERT_OK(Delete(1, "foo")); ASSERT_EQ(AllEntriesFor("foo", 1), "[ DEL, v3 ]"); ASSERT_OK(Flush(1)); ASSERT_EQ(AllEntriesFor("foo", 1), "[ DEL ]"); - dbfull()->CompactRange(CompactRangeOptions(), handles_[1], nullptr, - nullptr); + ASSERT_OK(dbfull()->CompactRange(CompactRangeOptions(), handles_[1], + nullptr, nullptr)); ASSERT_EQ(AllEntriesFor("foo", 1), "[ ]"); // Case 4: Put followed by another Put - Put(1, "foo", "v4"); - Put(1, "foo", "v5"); + ASSERT_OK(Put(1, "foo", "v4")); + ASSERT_OK(Put(1, "foo", "v5")); ASSERT_EQ(AllEntriesFor("foo", 1), "[ v5, v4 ]"); ASSERT_OK(Flush(1)); ASSERT_EQ(AllEntriesFor("foo", 1), "[ v5 ]"); - dbfull()->CompactRange(CompactRangeOptions(), handles_[1], nullptr, - nullptr); + ASSERT_OK(dbfull()->CompactRange(CompactRangeOptions(), handles_[1], + nullptr, nullptr)); ASSERT_EQ(AllEntriesFor("foo", 1), "[ v5 ]"); // clear database - Delete(1, "foo"); - dbfull()->CompactRange(CompactRangeOptions(), handles_[1], nullptr, - nullptr); + ASSERT_OK(Delete(1, "foo")); + ASSERT_OK(dbfull()->CompactRange(CompactRangeOptions(), handles_[1], + nullptr, nullptr)); ASSERT_EQ(AllEntriesFor("foo", 1), "[ ]"); // Case 5: Put followed by snapshot followed by another Put // Both puts should remain. - Put(1, "foo", "v6"); + ASSERT_OK(Put(1, "foo", "v6")); const Snapshot* snapshot = db_->GetSnapshot(); - Put(1, "foo", "v7"); + ASSERT_OK(Put(1, "foo", "v7")); ASSERT_OK(Flush(1)); ASSERT_EQ(AllEntriesFor("foo", 1), "[ v7, v6 ]"); db_->ReleaseSnapshot(snapshot); // clear database - Delete(1, "foo"); - dbfull()->CompactRange(CompactRangeOptions(), handles_[1], nullptr, - nullptr); + ASSERT_OK(Delete(1, "foo")); + ASSERT_OK(dbfull()->CompactRange(CompactRangeOptions(), handles_[1], + nullptr, nullptr)); ASSERT_EQ(AllEntriesFor("foo", 1), "[ ]"); // Case 5: snapshot followed by a put followed by another Put // Only the last put should remain. const Snapshot* snapshot1 = db_->GetSnapshot(); - Put(1, "foo", "v8"); - Put(1, "foo", "v9"); + ASSERT_OK(Put(1, "foo", "v8")); + ASSERT_OK(Put(1, "foo", "v9")); ASSERT_OK(Flush(1)); ASSERT_EQ(AllEntriesFor("foo", 1), "[ v9 ]"); db_->ReleaseSnapshot(snapshot1); @@ -891,7 +893,7 @@ TEST_F(DBBasicTest, FlushOneColumnFamily) { ASSERT_OK(Put(7, "popovich", "popovich")); for (int i = 0; i < 8; ++i) { - Flush(i); + ASSERT_OK(Flush(i)); auto tables = ListTableFiles(env_, dbname_); ASSERT_EQ(tables.size(), i + 1U); } @@ -1033,7 +1035,7 @@ class TestEnv : public EnvWrapper { explicit TestLogger(TestEnv* env_ptr) : Logger() { env = env_ptr; } ~TestLogger() override { if (!closed_) { - CloseHelper(); + CloseHelper().PermitUncheckedError(); } } void Logv(const char* /*format*/, va_list /*ap*/) override {} @@ -2231,12 +2233,14 @@ TEST_F(DBBasicTest, RecoverWithMissingFiles) { std::unique_ptr iter(db_->NewIterator(read_opts, handles_[0])); iter->SeekToFirst(); ASSERT_FALSE(iter->Valid()); + ASSERT_OK(iter->status()); iter.reset(db_->NewIterator(read_opts, handles_[1])); iter->SeekToFirst(); ASSERT_TRUE(iter->Valid()); ASSERT_EQ("a", iter->key()); iter->Next(); ASSERT_FALSE(iter->Valid()); + ASSERT_OK(iter->status()); iter.reset(db_->NewIterator(read_opts, handles_[2])); iter->SeekToFirst(); ASSERT_TRUE(iter->Valid()); @@ -2246,6 +2250,7 @@ TEST_F(DBBasicTest, RecoverWithMissingFiles) { ASSERT_EQ("b", iter->key()); iter->Next(); ASSERT_FALSE(iter->Valid()); + ASSERT_OK(iter->status()); } } @@ -2358,12 +2363,14 @@ TEST_F(DBBasicTest, SkipWALIfMissingTableFiles) { std::unique_ptr iter(db_->NewIterator(read_opts, handles_[0])); iter->SeekToFirst(); ASSERT_FALSE(iter->Valid()); + ASSERT_OK(iter->status()); iter.reset(db_->NewIterator(read_opts, handles_[1])); iter->SeekToFirst(); ASSERT_TRUE(iter->Valid()); ASSERT_EQ("a", iter->key()); iter->Next(); ASSERT_FALSE(iter->Valid()); + ASSERT_OK(iter->status()); } #endif // !ROCKSDB_LITE @@ -2893,9 +2900,8 @@ class DeadlineFS : public FileSystemWrapper { std::unique_ptr* result, IODebugContext* dbg) override { std::unique_ptr file; - IOStatus s; - - s = target()->NewRandomAccessFile(fname, opts, &file, dbg); + IOStatus s = target()->NewRandomAccessFile(fname, opts, &file, dbg); + EXPECT_OK(s); result->reset(new DeadlineRandomAccessFile(*this, file)); const std::chrono::microseconds deadline = GetDeadline(); @@ -3266,7 +3272,7 @@ TEST_P(DBBasicTestDeadline, PointLookupDeadline) { Random rnd(301); for (int i = 0; i < 400; ++i) { std::string key = "k" + ToString(i); - Put(key, rnd.RandomString(100)); + ASSERT_OK(Put(key, rnd.RandomString(100))); } Flush(); @@ -3349,9 +3355,9 @@ TEST_P(DBBasicTestDeadline, IteratorDeadline) { Random rnd(301); for (int i = 0; i < 400; ++i) { std::string key = "k" + ToString(i); - Put(key, rnd.RandomString(100)); + ASSERT_OK(Put(key, rnd.RandomString(100))); } - Flush(); + ASSERT_OK(Flush()); bool timedout = true; // A timeout will be forced when the IO counter reaches this value diff --git a/db/db_impl/db_impl_compaction_flush.cc b/db/db_impl/db_impl_compaction_flush.cc index 9115289d4..50783488b 100644 --- a/db/db_impl/db_impl_compaction_flush.cc +++ b/db/db_impl/db_impl_compaction_flush.cc @@ -126,10 +126,12 @@ IOStatus DBImpl::SyncClosedLogs(JobContext* job_context) { MarkLogsSynced(current_log_number - 1, true, io_s); if (!io_s.ok()) { if (total_log_size_ > 0) { - error_handler_.SetBGError(io_s, BackgroundErrorReason::kFlush); + error_handler_.SetBGError(io_s, BackgroundErrorReason::kFlush) + .PermitUncheckedError(); } else { // If the WAL is empty, we use different error reason - error_handler_.SetBGError(io_s, BackgroundErrorReason::kFlushNoWAL); + error_handler_.SetBGError(io_s, BackgroundErrorReason::kFlushNoWAL) + .PermitUncheckedError(); } TEST_SYNC_POINT("DBImpl::SyncClosedLogs:Failed"); return io_s; @@ -186,7 +188,8 @@ Status DBImpl::FlushMemTableToOutputFile( s = io_s; if (!io_s.ok() && !io_s.IsShutdownInProgress() && !io_s.IsColumnFamilyDropped()) { - error_handler_.SetBGError(io_s, BackgroundErrorReason::kFlush); + error_handler_.SetBGError(io_s, BackgroundErrorReason::kFlush) + .PermitUncheckedError(); } } else { TEST_SYNC_POINT("DBImpl::SyncClosedLogs:Skip"); @@ -246,16 +249,24 @@ Status DBImpl::FlushMemTableToOutputFile( // be pessimistic and try write to a new MANIFEST. // TODO: distinguish between MANIFEST write and CURRENT renaming if (!versions_->io_status().ok()) { - error_handler_.SetBGError(io_s, BackgroundErrorReason::kManifestWrite); + // Should handle return error? + error_handler_.SetBGError(io_s, BackgroundErrorReason::kManifestWrite) + .PermitUncheckedError(); } else if (total_log_size_ > 0) { - error_handler_.SetBGError(io_s, BackgroundErrorReason::kFlush); + // Should handle return error? + error_handler_.SetBGError(io_s, BackgroundErrorReason::kFlush) + .PermitUncheckedError(); } else { // If the WAL is empty, we use different error reason - error_handler_.SetBGError(io_s, BackgroundErrorReason::kFlushNoWAL); + // Should handle return error? + error_handler_.SetBGError(io_s, BackgroundErrorReason::kFlushNoWAL) + .PermitUncheckedError(); } } else { Status new_bg_error = s; - error_handler_.SetBGError(new_bg_error, BackgroundErrorReason::kFlush); + // Should handle return error? + error_handler_.SetBGError(new_bg_error, BackgroundErrorReason::kFlush) + .PermitUncheckedError(); } } else { // If we got here, then we decided not to care about the i_os status (either @@ -280,7 +291,9 @@ Status DBImpl::FlushMemTableToOutputFile( TEST_SYNC_POINT_CALLBACK( "DBImpl::FlushMemTableToOutputFile:MaxAllowedSpaceReached", &new_bg_error); - error_handler_.SetBGError(new_bg_error, BackgroundErrorReason::kFlush); + // Should handle this error? + error_handler_.SetBGError(new_bg_error, BackgroundErrorReason::kFlush) + .PermitUncheckedError(); } } #endif // ROCKSDB_LITE @@ -628,8 +641,9 @@ Status DBImpl::AtomicFlushMemTablesToOutputFiles( error_handler_.GetBGError().ok()) { Status new_bg_error = Status::SpaceLimit("Max allowed space was reached"); - error_handler_.SetBGError(new_bg_error, - BackgroundErrorReason::kFlush); + // Should Handle this error? + error_handler_.SetBGError(new_bg_error, BackgroundErrorReason::kFlush) + .PermitUncheckedError(); } } } @@ -646,16 +660,24 @@ Status DBImpl::AtomicFlushMemTablesToOutputFiles( // be pessimistic and try write to a new MANIFEST. // TODO: distinguish between MANIFEST write and CURRENT renaming if (!versions_->io_status().ok()) { - error_handler_.SetBGError(io_s, BackgroundErrorReason::kManifestWrite); + // Should Handle this error? + error_handler_.SetBGError(io_s, BackgroundErrorReason::kManifestWrite) + .PermitUncheckedError(); } else if (total_log_size_ > 0) { - error_handler_.SetBGError(io_s, BackgroundErrorReason::kFlush); + // Should Handle this error? + error_handler_.SetBGError(io_s, BackgroundErrorReason::kFlush) + .PermitUncheckedError(); } else { // If the WAL is empty, we use different error reason - error_handler_.SetBGError(io_s, BackgroundErrorReason::kFlushNoWAL); + // Should Handle this error? + error_handler_.SetBGError(io_s, BackgroundErrorReason::kFlushNoWAL) + .PermitUncheckedError(); } } else { Status new_bg_error = s; - error_handler_.SetBGError(new_bg_error, BackgroundErrorReason::kFlush); + // Should Handle this error? + error_handler_.SetBGError(new_bg_error, BackgroundErrorReason::kFlush) + .PermitUncheckedError(); } } @@ -1178,9 +1200,11 @@ Status DBImpl::CompactFilesImpl( job_context->job_id, status.ToString().c_str()); IOStatus io_s = compaction_job.io_status(); if (!io_s.ok()) { - error_handler_.SetBGError(io_s, BackgroundErrorReason::kCompaction); + error_handler_.SetBGError(io_s, BackgroundErrorReason::kCompaction) + .PermitUncheckedError(); } else { - error_handler_.SetBGError(status, BackgroundErrorReason::kCompaction); + error_handler_.SetBGError(status, BackgroundErrorReason::kCompaction) + .PermitUncheckedError(); } } @@ -2928,7 +2952,8 @@ Status DBImpl::BackgroundCompaction(bool* made_progress, mutex_.Unlock(); TEST_SYNC_POINT_CALLBACK( "DBImpl::BackgroundCompaction:NonTrivial:BeforeRun", nullptr); - compaction_job.Run(); + // Should handle erorr? + compaction_job.Run().PermitUncheckedError(); TEST_SYNC_POINT("DBImpl::BackgroundCompaction:NonTrivial:AfterRun"); mutex_.Lock(); @@ -2946,6 +2971,8 @@ Status DBImpl::BackgroundCompaction(bool* made_progress, if (status.ok() && !io_s.ok()) { status = io_s; + } else { + io_s.PermitUncheckedError(); } if (c != nullptr) { @@ -2982,9 +3009,10 @@ Status DBImpl::BackgroundCompaction(bool* made_progress, auto err_reason = versions_->io_status().ok() ? BackgroundErrorReason::kCompaction : BackgroundErrorReason::kManifestWrite; - error_handler_.SetBGError(io_s, err_reason); + error_handler_.SetBGError(io_s, err_reason).PermitUncheckedError(); } else { - error_handler_.SetBGError(status, BackgroundErrorReason::kCompaction); + error_handler_.SetBGError(status, BackgroundErrorReason::kCompaction) + .PermitUncheckedError(); } if (c != nullptr && !is_manual && !error_handler_.IsBGWorkStopped()) { // Put this cfd back in the compaction queue so we can retry after some diff --git a/db/db_impl/db_impl_files.cc b/db/db_impl/db_impl_files.cc index 0953bf3da..ee8494919 100644 --- a/db/db_impl/db_impl_files.cc +++ b/db/db_impl/db_impl_files.cc @@ -215,7 +215,8 @@ void DBImpl::FindObsoleteFiles(JobContext* job_context, bool force, if (immutable_db_options_.wal_dir != dbname_) { std::vector log_files; env_->GetChildren(immutable_db_options_.wal_dir, - &log_files); // Ignore errors + &log_files) + .PermitUncheckedError(); // Ignore errors for (const std::string& log_file : log_files) { job_context->full_scan_candidate_files.emplace_back( log_file, immutable_db_options_.wal_dir); @@ -226,7 +227,8 @@ void DBImpl::FindObsoleteFiles(JobContext* job_context, bool force, immutable_db_options_.db_log_dir != dbname_) { std::vector info_log_files; // Ignore errors - env_->GetChildren(immutable_db_options_.db_log_dir, &info_log_files); + env_->GetChildren(immutable_db_options_.db_log_dir, &info_log_files) + .PermitUncheckedError(); for (std::string& log_file : info_log_files) { job_context->full_scan_candidate_files.emplace_back( log_file, immutable_db_options_.db_log_dir); @@ -762,7 +764,8 @@ Status DBImpl::FinishBestEffortsRecovery() { std::set files_to_delete; for (const auto& path : paths) { std::vector files; - env_->GetChildren(path, &files); + // Should we handle it? + env_->GetChildren(path, &files).PermitUncheckedError(); for (const auto& fname : files) { uint64_t number = 0; FileType type; diff --git a/db/db_impl/db_impl_open.cc b/db/db_impl/db_impl_open.cc index b3cd8dd8e..9e3a5b549 100644 --- a/db/db_impl/db_impl_open.cc +++ b/db/db_impl/db_impl_open.cc @@ -1757,6 +1757,10 @@ Status DBImpl::Open(const DBOptions& db_options, const std::string& dbname, "DB::Open() failed --- Unable to persist Options file", persist_options_status.ToString()); } + } else { + ROCKS_LOG_WARN(impl->immutable_db_options_.info_log, + "Persisting Option File error: %s", + persist_options_status.ToString().c_str()); } if (s.ok()) { impl->StartStatsDumpScheduler(); diff --git a/db/db_impl/db_impl_write.cc b/db/db_impl/db_impl_write.cc index 633090be9..735b9975a 100644 --- a/db/db_impl/db_impl_write.cc +++ b/db/db_impl/db_impl_write.cc @@ -101,12 +101,10 @@ Status DBImpl::WriteImpl(const WriteOptions& write_options, assert(!WriteBatchInternal::IsLatestPersistentState(my_batch) || disable_memtable); - Status status; - IOStatus io_s; if (write_options.low_pri) { - status = ThrottleLowPriWritesIfNeeded(write_options, my_batch); - if (!status.ok()) { - return status; + Status s = ThrottleLowPriWritesIfNeeded(write_options, my_batch); + if (!s.ok()) { + return s; } } @@ -129,10 +127,10 @@ Status DBImpl::WriteImpl(const WriteOptions& write_options, uint64_t seq; // Use a write thread to i) optimize for WAL write, ii) publish last // sequence in in increasing order, iii) call pre_release_callback serially - status = WriteImplWALOnly(&write_thread_, write_options, my_batch, callback, - log_used, log_ref, &seq, sub_batch_cnt, - pre_release_callback, kDoAssignOrder, - kDoPublishLastSeq, disable_memtable); + Status status = WriteImplWALOnly( + &write_thread_, write_options, my_batch, callback, log_used, log_ref, + &seq, sub_batch_cnt, pre_release_callback, kDoAssignOrder, + kDoPublishLastSeq, disable_memtable); TEST_SYNC_POINT("DBImpl::WriteImpl:UnorderedWriteAfterWriteWAL"); if (!status.ok()) { return status; @@ -164,6 +162,7 @@ Status DBImpl::WriteImpl(const WriteOptions& write_options, StopWatch write_sw(env_, immutable_db_options_.statistics.get(), DB_WRITE); write_thread_.JoinBatchGroup(&w); + Status status; if (w.state == WriteThread::STATE_PARALLEL_MEMTABLE_WRITER) { // we are a non-leader in a parallel group @@ -204,6 +203,8 @@ Status DBImpl::WriteImpl(const WriteOptions& write_options, *seq_used = w.sequence; } // write is complete and leader has updated sequence + // Should we handle it? + status.PermitUncheckedError(); return w.FinalStatus(); } // else we are the leader of the write batch group @@ -252,6 +253,7 @@ Status DBImpl::WriteImpl(const WriteOptions& write_options, last_batch_group_size_ = write_thread_.EnterAsBatchGroupLeader(&w, &write_group); + IOStatus io_s; if (status.ok()) { // Rules for when we can update the memtable concurrently // 1. supported by memtable @@ -1899,7 +1901,10 @@ Status DB::Delete(const WriteOptions& opt, ColumnFamilyHandle* column_family, Status DB::SingleDelete(const WriteOptions& opt, ColumnFamilyHandle* column_family, const Slice& key) { WriteBatch batch; - batch.SingleDelete(column_family, key); + Status s = batch.SingleDelete(column_family, key); + if (!s.ok()) { + return s; + } return Write(opt, &batch); } @@ -1907,7 +1912,10 @@ Status DB::DeleteRange(const WriteOptions& opt, ColumnFamilyHandle* column_family, const Slice& begin_key, const Slice& end_key) { WriteBatch batch; - batch.DeleteRange(column_family, begin_key, end_key); + Status s = batch.DeleteRange(column_family, begin_key, end_key); + if (!s.ok()) { + return s; + } return Write(opt, &batch); } diff --git a/db/db_iter.h b/db/db_iter.h index c3999ef1e..93b78c06e 100644 --- a/db/db_iter.h +++ b/db/db_iter.h @@ -140,7 +140,14 @@ class DBIter final : public Iterator { } ReadRangeDelAggregator* GetRangeDelAggregator() { return &range_del_agg_; } - bool Valid() const override { return valid_; } + bool Valid() const override { +#ifdef ROCKSDB_ASSERT_STATUS_CHECKED + if (valid_) { + status_.PermitUncheckedError(); + } +#endif // ROCKSDB_ASSERT_STATUS_CHECKED + return valid_; + } Slice key() const override { assert(valid_); if (start_seqnum_ > 0 || timestamp_lb_) { diff --git a/db/db_test_util.cc b/db/db_test_util.cc index 8effdb5f7..3ad1f31e4 100644 --- a/db/db_test_util.cc +++ b/db/db_test_util.cc @@ -1182,9 +1182,9 @@ void DBTestBase::FillLevels(const std::string& smallest, void DBTestBase::MoveFilesToLevel(int level, int cf) { for (int l = 0; l < level; ++l) { if (cf > 0) { - dbfull()->TEST_CompactRange(l, nullptr, nullptr, handles_[cf]); + EXPECT_OK(dbfull()->TEST_CompactRange(l, nullptr, nullptr, handles_[cf])); } else { - dbfull()->TEST_CompactRange(l, nullptr, nullptr); + EXPECT_OK(dbfull()->TEST_CompactRange(l, nullptr, nullptr)); } } } @@ -1436,7 +1436,7 @@ std::vector DBTestBase::ListTableFiles(Env* env, const std::string& path) { std::vector files; std::vector file_numbers; - env->GetChildren(path, &files); + EXPECT_OK(env->GetChildren(path, &files)); uint64_t number; FileType type; for (size_t i = 0; i < files.size(); ++i) { diff --git a/db/error_handler.cc b/db/error_handler.cc index 7dae0a638..d44af490f 100644 --- a/db/error_handler.cc +++ b/db/error_handler.cc @@ -338,7 +338,7 @@ Status ErrorHandler::SetBGError(const IOStatus& bg_io_err, } if (BackgroundErrorReason::kManifestWrite == reason) { // Always returns ok - db_->DisableFileDeletionsWithLock(); + db_->DisableFileDeletionsWithLock().PermitUncheckedError(); } Status new_bg_io_err = bg_io_err; diff --git a/db/table_cache.cc b/db/table_cache.cc index 0a1076e2c..1f760c905 100644 --- a/db/table_cache.cc +++ b/db/table_cache.cc @@ -162,7 +162,6 @@ Status TableCache::FindTable(const ReadOptions& ro, int level, bool prefetch_index_and_filter_in_cache, size_t max_file_size_for_l0_meta_pin) { PERF_TIMER_GUARD_WITH_ENV(find_table_nanos, ioptions_.env); - Status s; uint64_t number = fd.GetNumber(); Slice key = GetSliceForFileNumber(&number); *handle = cache_->Lookup(key); @@ -177,15 +176,15 @@ Status TableCache::FindTable(const ReadOptions& ro, // We check the cache again under loading mutex *handle = cache_->Lookup(key); if (*handle != nullptr) { - return s; + return Status::OK(); } std::unique_ptr table_reader; - s = GetTableReader(ro, file_options, internal_comparator, fd, - false /* sequential mode */, record_read_stats, - file_read_hist, &table_reader, prefix_extractor, - skip_filters, level, prefetch_index_and_filter_in_cache, - max_file_size_for_l0_meta_pin); + Status s = GetTableReader( + ro, file_options, internal_comparator, fd, false /* sequential mode */, + record_read_stats, file_read_hist, &table_reader, prefix_extractor, + skip_filters, level, prefetch_index_and_filter_in_cache, + max_file_size_for_l0_meta_pin); if (!s.ok()) { assert(table_reader == nullptr); RecordTick(ioptions_.statistics, NO_FILE_ERRORS); @@ -199,8 +198,9 @@ Status TableCache::FindTable(const ReadOptions& ro, table_reader.release(); } } + return s; } - return s; + return Status::OK(); } InternalIterator* TableCache::NewIterator( @@ -413,7 +413,8 @@ Status TableCache::Get(const ReadOptions& options, Status s; TableReader* t = fd.table_reader; Cache::Handle* handle = nullptr; - if (!done && s.ok()) { + if (!done) { + assert(s.ok()); if (t == nullptr) { s = FindTable(options, file_options_, internal_comparator, fd, &handle, prefix_extractor, @@ -455,8 +456,11 @@ Status TableCache::Get(const ReadOptions& options, size_t charge = row_cache_key.Size() + row_cache_entry->size() + sizeof(std::string); void* row_ptr = new std::string(std::move(*row_cache_entry)); - ioptions_.row_cache->Insert(row_cache_key.GetUserKey(), row_ptr, charge, - &DeleteEntry); + // If row cache is full, it's OK to continue. + ioptions_.row_cache + ->Insert(row_cache_key.GetUserKey(), row_ptr, charge, + &DeleteEntry) + .PermitUncheckedError(); } #endif // ROCKSDB_LITE @@ -575,8 +579,11 @@ Status TableCache::MultiGet(const ReadOptions& options, size_t charge = row_cache_key.Size() + row_cache_entry.size() + sizeof(std::string); void* row_ptr = new std::string(std::move(row_cache_entry)); - ioptions_.row_cache->Insert(row_cache_key.GetUserKey(), row_ptr, charge, - &DeleteEntry); + // If row cache is full, it's OK. + ioptions_.row_cache + ->Insert(row_cache_key.GetUserKey(), row_ptr, charge, + &DeleteEntry) + .PermitUncheckedError(); } } } @@ -593,18 +600,17 @@ Status TableCache::GetTableProperties( const InternalKeyComparator& internal_comparator, const FileDescriptor& fd, std::shared_ptr* properties, const SliceTransform* prefix_extractor, bool no_io) { - Status s; auto table_reader = fd.table_reader; // table already been pre-loaded? if (table_reader) { *properties = table_reader->GetTableProperties(); - return s; + return Status::OK(); } Cache::Handle* table_handle = nullptr; - s = FindTable(ReadOptions(), file_options, internal_comparator, fd, - &table_handle, prefix_extractor, no_io); + Status s = FindTable(ReadOptions(), file_options, internal_comparator, fd, + &table_handle, prefix_extractor, no_io); if (!s.ok()) { return s; } diff --git a/db/version_builder.cc b/db/version_builder.cc index da500d5e0..5e7fe540c 100644 --- a/db/version_builder.cc +++ b/db/version_builder.cc @@ -962,6 +962,11 @@ class VersionBuilder::Rep { for (auto& t : threads) { t.join(); } +#ifdef ROCKSDB_ASSERT_STATUS_CHECKED + for (const auto& s : statuses) { + s.PermitUncheckedError(); + } +#endif // ROCKSDB_ASSERT_STATUS_CHECKED for (const auto& s : statuses) { if (!s.ok()) { return s; diff --git a/db/version_set.cc b/db/version_set.cc index e94f5870d..471bf1da5 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -1836,6 +1836,8 @@ void Version::Get(const ReadOptions& read_options, const LookupKey& k, GetPerfLevel() >= PerfLevel::kEnableTimeExceptForMutex && get_perf_context()->per_level_perf_context_enabled; StopWatchNano timer(env_, timer_enabled /* auto_start */); + // Something feels not right here. Should investigate more. + status->PermitUncheckedError(); *status = table_cache_->Get( read_options, *internal_comparator(), *f->file_metadata, ikey, &get_context, mutable_cf_options_.prefix_extractor.get(), diff --git a/table/block_based/block_based_table_builder.cc b/table/block_based/block_based_table_builder.cc index b58dbe68b..53ebfead2 100644 --- a/table/block_based/block_based_table_builder.cc +++ b/table/block_based/block_based_table_builder.cc @@ -496,7 +496,12 @@ struct BlockBasedTableBuilder::Rep { Rep(const Rep&) = delete; Rep& operator=(const Rep&) = delete; - ~Rep() {} + ~Rep() { + // They are supposed to be passed back to users through Finish() + // if the file finishes. + status.PermitUncheckedError(); + io_status.PermitUncheckedError(); + } private: Status status; @@ -1090,6 +1095,7 @@ void BlockBasedTableBuilder::WriteRawBlock(const Slice& block_contents, static_cast(trailer)); io_s = r->file->Append(Slice(trailer, kBlockTrailerSize)); if (io_s.ok()) { + assert(s.ok()); s = InsertBlockInCache(block_contents, type, handle); if (!s.ok()) { r->SetStatus(s); @@ -1264,13 +1270,16 @@ Status BlockBasedTableBuilder::InsertBlockInCache(const Slice& block_contents, static_cast(end - r->compressed_cache_key_prefix)); // Insert into compressed block cache. - block_cache_compressed->Insert( - key, block_contents_to_cache, - block_contents_to_cache->ApproximateMemoryUsage(), - &DeleteCachedBlockContents); + // How should we deal with compressed cache full? + block_cache_compressed + ->Insert(key, block_contents_to_cache, + block_contents_to_cache->ApproximateMemoryUsage(), + &DeleteCachedBlockContents) + .PermitUncheckedError(); // Invalidate OS cache. - r->file->InvalidateCache(static_cast(r->get_offset()), size); + r->file->InvalidateCache(static_cast(r->get_offset()), size) + .PermitUncheckedError(); } return Status::OK(); } @@ -1677,7 +1686,9 @@ Status BlockBasedTableBuilder::Finish() { WriteFooter(metaindex_block_handle, index_block_handle); } r->state = Rep::State::kClosed; - return r->GetStatus(); + Status ret_status = r->GetStatus(); + assert(!ret_status.ok() || io_status().ok()); + return ret_status; } void BlockBasedTableBuilder::Abandon() { diff --git a/table/block_based/block_based_table_reader.cc b/table/block_based/block_based_table_reader.cc index fbc701d5e..a98167f1c 100644 --- a/table/block_based/block_based_table_reader.cc +++ b/table/block_based/block_based_table_reader.cc @@ -1692,7 +1692,9 @@ void BlockBasedTable::RetrieveMultipleBlocks( req.status = s; } } else { - file->MultiRead(opts, &read_reqs[0], read_reqs.size(), &direct_io_buf); + // How to handle this status code? + file->MultiRead(opts, &read_reqs[0], read_reqs.size(), &direct_io_buf) + .PermitUncheckedError(); } } @@ -1800,6 +1802,7 @@ void BlockBasedTable::RetrieveMultipleBlocks( // block cache is configured. In that case, fall // through and set up the block explicitly if (block_entry->GetValue() != nullptr) { + s.PermitUncheckedError(); continue; } } @@ -2440,6 +2443,7 @@ void BlockBasedTable::MultiGet(const ReadOptions& read_options, CachableEntry uncompression_dict; Status uncompression_dict_status; + uncompression_dict_status.PermitUncheckedError(); bool uncompression_dict_inited = false; size_t total_len = 0; ReadOptions ro = read_options; @@ -2707,6 +2711,12 @@ void BlockBasedTable::MultiGet(const ReadOptions& read_options, } *(miter->s) = s; } +#ifdef ROCKSDB_ASSERT_STATUS_CHECKED + // Not sure why we need to do it. Should investigate more. + for (auto& st : statuses) { + st.PermitUncheckedError(); + } +#endif // ROCKSDB_ASSERT_STATUS_CHECKED } } diff --git a/table/block_based/index_builder.h b/table/block_based/index_builder.h index bfffc5996..23e4a76d5 100644 --- a/table/block_based/index_builder.h +++ b/table/block_based/index_builder.h @@ -307,12 +307,13 @@ class HashIndexBuilder : public IndexBuilder { if (pending_block_num_ != 0) { FlushPendingPrefix(); } - primary_index_builder_.Finish(index_blocks, last_partition_block_handle); + Status s = primary_index_builder_.Finish(index_blocks, + last_partition_block_handle); index_blocks->meta_blocks.insert( {kHashIndexPrefixesBlock.c_str(), prefix_block_}); index_blocks->meta_blocks.insert( {kHashIndexPrefixesMetadataBlock.c_str(), prefix_meta_block_}); - return Status::OK(); + return s; } virtual size_t IndexSize() const override { diff --git a/table/plain/plain_table_builder.cc b/table/plain/plain_table_builder.cc index 0eed5b86b..e3ef67da6 100644 --- a/table/plain/plain_table_builder.cc +++ b/table/plain/plain_table_builder.cc @@ -116,6 +116,10 @@ PlainTableBuilder::PlainTableBuilder( } PlainTableBuilder::~PlainTableBuilder() { + // They are supposed to have been passed to users through Finish() + // if the file succeeds. + status_.PermitUncheckedError(); + io_status_.PermitUncheckedError(); } void PlainTableBuilder::Add(const Slice& key, const Slice& value) { diff --git a/table/plain/plain_table_key_coding.cc b/table/plain/plain_table_key_coding.cc index 6c7b3c751..0f3b37fd1 100644 --- a/table/plain/plain_table_key_coding.cc +++ b/table/plain/plain_table_key_coding.cc @@ -487,7 +487,6 @@ Status PlainTableKeyDecoder::NextKeyNoValue(uint32_t start_offset, if (seekable != nullptr) { *seekable = true; } - Status s; if (encoding_type_ == kPlain) { return NextPlainEncodingKey(start_offset, parsed_key, internal_key, bytes_read, seekable); diff --git a/table/plain/plain_table_key_coding.h b/table/plain/plain_table_key_coding.h index 2c345ad73..1543dec5b 100644 --- a/table/plain/plain_table_key_coding.h +++ b/table/plain/plain_table_key_coding.h @@ -68,6 +68,12 @@ class PlainTableFileReader { public: explicit PlainTableFileReader(const PlainTableReaderFileInfo* _file_info) : file_info_(_file_info), num_buf_(0) {} + + ~PlainTableFileReader() { + // Should fix. + status_.PermitUncheckedError(); + } + // In mmaped mode, the results point to mmaped area of the file, which // means it is always valid before closing the file. // In non-mmap mode, the results point to an internal buffer. If the caller @@ -146,6 +152,7 @@ class PlainTableKeyDecoder { fixed_user_key_len_(user_key_len), prefix_extractor_(prefix_extractor), in_prefix_(false) {} + // Find the next key. // start: char array where the key starts. // limit: boundary of the char array diff --git a/table/plain/plain_table_reader.cc b/table/plain/plain_table_reader.cc index 8700587bc..0b734308b 100644 --- a/table/plain/plain_table_reader.cc +++ b/table/plain/plain_table_reader.cc @@ -113,6 +113,8 @@ PlainTableReader::PlainTableReader( table_properties_(nullptr) {} PlainTableReader::~PlainTableReader() { + // Should fix? + status_.PermitUncheckedError(); } Status PlainTableReader::Open( diff --git a/utilities/fault_injection_env.h b/utilities/fault_injection_env.h index b1e8faa41..30e900328 100644 --- a/utilities/fault_injection_env.h +++ b/utilities/fault_injection_env.h @@ -212,6 +212,7 @@ class FaultInjectionTestEnv : public EnvWrapper { } void SetFilesystemActiveNoLock(bool active, Status error = Status::Corruption("Not active")) { + error.PermitUncheckedError(); filesystem_active_ = active; if (!active) { error_ = error; @@ -219,6 +220,7 @@ class FaultInjectionTestEnv : public EnvWrapper { } void SetFilesystemActive(bool active, Status error = Status::Corruption("Not active")) { + error.PermitUncheckedError(); MutexLock l(&mutex_); SetFilesystemActiveNoLock(active, error); }