From 5d16325ce33384de974560648434473031639018 Mon Sep 17 00:00:00 2001 From: Levi Tamasi Date: Sat, 3 Oct 2020 18:36:16 -0700 Subject: [PATCH] Revert "Return error if Get() fails in Prefetching Filter blocks (#7463)" (#7505) Summary: This reverts commit 7d503e66a9a9d9bdf17e2d35038bfe8f3e12f5dc. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7505 Reviewed By: ajkr Differential Revision: D24100875 Pulled By: ltamasi fbshipit-source-id: 8705e3e6e8be4b4fd175ffdb031baa6530b61151 --- file/file_prefetch_buffer.cc | 12 ++++----- table/block_based/block_based_table_reader.cc | 9 +++---- table/block_based/filter_block.h | 4 +-- table/block_based/partitioned_filter_block.cc | 27 ++++++++----------- table/block_based/partitioned_filter_block.h | 2 +- 5 files changed, 21 insertions(+), 33 deletions(-) diff --git a/file/file_prefetch_buffer.cc b/file/file_prefetch_buffer.cc index d813a7bb3..8d9798d09 100644 --- a/file/file_prefetch_buffer.cc +++ b/file/file_prefetch_buffer.cc @@ -91,19 +91,17 @@ Status FilePrefetchBuffer::Prefetch(const IOOptions& opts, size_t read_len = static_cast(roundup_len - chunk_len); s = reader->Read(opts, rounddown_offset + chunk_len, read_len, &result, buffer_.BufferStart() + chunk_len, nullptr, for_compaction); - if (!s.ok()) { - return s; - } - #ifndef NDEBUG - if (result.size() < read_len) { + if (!s.ok() || result.size() < read_len) { // Fake an IO error to force db_stress fault injection to ignore // truncated read errors IGNORE_STATUS_IF_ERROR(Status::IOError()); } #endif - buffer_offset_ = rounddown_offset; - buffer_.Size(static_cast(chunk_len) + result.size()); + if (s.ok()) { + buffer_offset_ = rounddown_offset; + buffer_.Size(static_cast(chunk_len) + result.size()); + } return s; } diff --git a/table/block_based/block_based_table_reader.cc b/table/block_based/block_based_table_reader.cc index d215174c6..dc4f6139c 100644 --- a/table/block_based/block_based_table_reader.cc +++ b/table/block_based/block_based_table_reader.cc @@ -1038,16 +1038,13 @@ Status BlockBasedTable::PrefetchIndexAndFilterBlocks( auto filter = new_table->CreateFilterBlockReader( ro, prefetch_buffer, use_cache, prefetch_filter, pin_filter, lookup_context); - if (filter) { - rep_->filter = std::move(filter); // Refer to the comment above about paritioned indexes always being cached if (prefetch_all) { - s = rep_->filter->CacheDependencies(ro, pin_all); - if (!s.ok()) { - return s; - } + filter->CacheDependencies(ro, pin_all); } + + rep_->filter = std::move(filter); } } diff --git a/table/block_based/filter_block.h b/table/block_based/filter_block.h index c0e4f300c..d94c7e606 100644 --- a/table/block_based/filter_block.h +++ b/table/block_based/filter_block.h @@ -153,9 +153,7 @@ class FilterBlockReader { return error_msg; } - virtual Status CacheDependencies(const ReadOptions& /*ro*/, bool /*pin*/) { - return Status::OK(); - } + virtual void CacheDependencies(const ReadOptions& /*ro*/, bool /*pin*/) {} virtual bool RangeMayExist(const Slice* /*iterate_upper_bound*/, const Slice& user_key, diff --git a/table/block_based/partitioned_filter_block.cc b/table/block_based/partitioned_filter_block.cc index 923436645..dc25abbea 100644 --- a/table/block_based/partitioned_filter_block.cc +++ b/table/block_based/partitioned_filter_block.cc @@ -412,8 +412,8 @@ size_t PartitionedFilterBlockReader::ApproximateMemoryUsage() const { } // TODO(myabandeh): merge this with the same function in IndexReader -Status PartitionedFilterBlockReader::CacheDependencies(const ReadOptions& ro, - bool pin) { +void PartitionedFilterBlockReader::CacheDependencies(const ReadOptions& ro, + bool pin) { assert(table()); const BlockBasedTable::Rep* const rep = table()->get_rep(); @@ -426,11 +426,12 @@ Status PartitionedFilterBlockReader::CacheDependencies(const ReadOptions& ro, Status s = GetOrReadFilterBlock(false /* no_io */, nullptr /* get_context */, &lookup_context, &filter_block); if (!s.ok()) { - ROCKS_LOG_ERROR(rep->ioptions.info_log, - "Error retrieving top-level filter block while trying to " - "cache filter partitions: %s", - s.ToString().c_str()); - return s; + ROCKS_LOG_WARN(rep->ioptions.info_log, + "Error retrieving top-level filter block while trying to " + "cache filter partitions: %s", + s.ToString().c_str()); + IGNORE_STATUS_IF_ERROR(s); + return; } // Before read partitions, prefetch them to avoid lots of IOs @@ -464,9 +465,6 @@ Status PartitionedFilterBlockReader::CacheDependencies(const ReadOptions& ro, s = prefetch_buffer->Prefetch(opts, rep->file.get(), prefetch_off, static_cast(prefetch_len)); } - if (!s.ok()) { - return s; - } // After prefetch, read the partitions one by one for (biter.SeekToFirst(); biter.Valid(); biter.Next()) { @@ -479,20 +477,17 @@ Status PartitionedFilterBlockReader::CacheDependencies(const ReadOptions& ro, prefetch_buffer.get(), ro, handle, UncompressionDict::GetEmptyDict(), &block, BlockType::kFilter, nullptr /* get_context */, &lookup_context, nullptr /* contents */); - if (!s.ok()) { - return s; - } - assert(s.ok() || block.GetValue() == nullptr); - if (block.GetValue() != nullptr) { + assert(s.ok() || block.GetValue() == nullptr); + if (s.ok() && block.GetValue() != nullptr) { if (block.IsCached()) { if (pin) { filter_map_[handle.offset()] = std::move(block); } } } + IGNORE_STATUS_IF_ERROR(s); } - return biter.status(); } const InternalKeyComparator* PartitionedFilterBlockReader::internal_comparator() diff --git a/table/block_based/partitioned_filter_block.h b/table/block_based/partitioned_filter_block.h index f6fde3338..2ccc8f8bc 100644 --- a/table/block_based/partitioned_filter_block.h +++ b/table/block_based/partitioned_filter_block.h @@ -130,7 +130,7 @@ class PartitionedFilterBlockReader : public FilterBlockReaderCommon { uint64_t block_offset, BlockHandle filter_handle, bool no_io, BlockCacheLookupContext* lookup_context, FilterManyFunction filter_function) const; - Status CacheDependencies(const ReadOptions& ro, bool pin) override; + void CacheDependencies(const ReadOptions& ro, bool pin) override; const InternalKeyComparator* internal_comparator() const; bool index_key_includes_seq() const;