kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From a...@apache.org
Subject kudu git commit: cfile_set: don't reset BloomFileReader on error
Date Fri, 03 Nov 2017 18:44:41 GMT
Repository: kudu
Updated Branches:
  refs/heads/master 10f6164b1 -> e2509a4bd


cfile_set: don't reset BloomFileReader on error

There is a race between UpdateCompactionStats and the error path of
CFileSet::FindRow(). When FindRow() fails to check its BloomFileReader,
it clears it, but the reader is necessary to determine the rowset size.
Both access the bloom reader without locks.

The reset is an optimization to allow the CFileSet to be scanned
post-error without having to read the bloom file. This isn't compelling:
the failure of a bloom reader doesn't affect the correctness of a scan,
and if the error is a permanent error (e.g. a disk failure), it would be
desirable to eventually fail and replace the tablet; if the error is
transient, then re-reading the bloom filter could yield successful
scans. In either case, clearing the reader doesn't seem necessary.

This patch removes the BloomFileReader reset on error. A couple of
places previously checked the presence of the bloom reader because of
the reset; this is no longer necessary. Also includes some updates to
style in a few related places.

Change-Id: I7435da448a1a33ca5216923311ab920f7a859806
Reviewed-on: http://gerrit.cloudera.org:8080/8440
Reviewed-by: Adar Dembo <adar@cloudera.com>
Tested-by: Adar Dembo <adar@cloudera.com>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/e2509a4b
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/e2509a4b
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/e2509a4b

Branch: refs/heads/master
Commit: e2509a4bdd807d83535e28f94a3986c4510aa7df
Parents: 10f6164
Author: Andrew Wong <awong@cloudera.com>
Authored: Thu Oct 26 12:35:58 2017 -0700
Committer: Adar Dembo <adar@cloudera.com>
Committed: Fri Nov 3 18:44:02 2017 +0000

----------------------------------------------------------------------
 src/kudu/cfile/bloomfile-test-base.h |  9 ++++-----
 src/kudu/cfile/bloomfile-test.cc     |  3 +--
 src/kudu/cfile/bloomfile.cc          | 10 ++++------
 src/kudu/cfile/bloomfile.h           | 18 ++++++++----------
 src/kudu/tablet/cfile_set.cc         | 27 +++++++++++++++------------
 src/kudu/tablet/cfile_set.h          |  2 +-
 6 files changed, 33 insertions(+), 36 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/e2509a4b/src/kudu/cfile/bloomfile-test-base.h
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/bloomfile-test-base.h b/src/kudu/cfile/bloomfile-test-base.h
index cf45ce4..a702d32 100644
--- a/src/kudu/cfile/bloomfile-test-base.h
+++ b/src/kudu/cfile/bloomfile-test-base.h
@@ -14,8 +14,8 @@
 // KIND, either express or implied.  See the License for the
 // specific language governing permissions and limitations
 // under the License.
-#ifndef KUDU_CFILE_BLOOMFILE_TEST_BASE_H
-#define KUDU_CFILE_BLOOMFILE_TEST_BASE_H
+
+#pragma once
 
 #include <glog/logging.h>
 #include <gtest/gtest.h>
@@ -120,12 +120,11 @@ class BloomFileTestBase : public KuduTest {
   }
 
  protected:
-  gscoped_ptr<FsManager> fs_manager_;
-  gscoped_ptr<BloomFileReader> bfr_;
+  std::unique_ptr<FsManager> fs_manager_;
+  std::unique_ptr<BloomFileReader> bfr_;
   BlockId block_id_;
 };
 
 } // namespace cfile
 } // namespace kudu
 
-#endif

http://git-wip-us.apache.org/repos/asf/kudu/blob/e2509a4b/src/kudu/cfile/bloomfile-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/bloomfile-test.cc b/src/kudu/cfile/bloomfile-test.cc
index 42f16c3..f699b01 100644
--- a/src/kudu/cfile/bloomfile-test.cc
+++ b/src/kudu/cfile/bloomfile-test.cc
@@ -31,7 +31,6 @@
 #include "kudu/fs/fs-test-util.h"
 #include "kudu/fs/fs_manager.h"
 #include "kudu/gutil/endian.h"
-#include "kudu/gutil/gscoped_ptr.h"
 #include "kudu/util/bloom_filter.h"
 #include "kudu/util/mem_tracker.h"
 #include "kudu/util/slice.h"
@@ -123,7 +122,7 @@ TEST_F(BloomFileTest, TestLazyInit) {
 
   // Lazily opening the bloom file should not trigger any reads,
   // and the file size should be available before Init().
-  gscoped_ptr<BloomFileReader> reader;
+  unique_ptr<BloomFileReader> reader;
   ReaderOptions opts;
   opts.parent_mem_tracker = tracker;
   ASSERT_OK(BloomFileReader::OpenNoInit(std::move(count_block), opts, &reader));

http://git-wip-us.apache.org/repos/asf/kudu/blob/e2509a4b/src/kudu/cfile/bloomfile.cc
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/bloomfile.cc b/src/kudu/cfile/bloomfile.cc
index 4277045..2f13525 100644
--- a/src/kudu/cfile/bloomfile.cc
+++ b/src/kudu/cfile/bloomfile.cc
@@ -18,7 +18,6 @@
 #include <cstdint>
 #include <ostream>
 #include <string>
-#include <type_traits>
 #include <utility>
 #include <vector>
 
@@ -37,7 +36,6 @@
 #include "kudu/common/types.h"
 #include "kudu/fs/block_manager.h"
 #include "kudu/gutil/atomicops.h"
-#include "kudu/gutil/move.h"
 #include "kudu/gutil/port.h"
 #include "kudu/gutil/stringprintf.h"
 #include "kudu/util/coding.h"
@@ -196,8 +194,8 @@ Status BloomFileWriter::FinishCurrentBloomBlock() {
 
 Status BloomFileReader::Open(unique_ptr<ReadableBlock> block,
                              ReaderOptions options,
-                             gscoped_ptr<BloomFileReader> *reader) {
-  gscoped_ptr<BloomFileReader> bf_reader;
+                             unique_ptr<BloomFileReader> *reader) {
+  unique_ptr<BloomFileReader> bf_reader;
   RETURN_NOT_OK(OpenNoInit(std::move(block),
                            std::move(options), &bf_reader));
   RETURN_NOT_OK(bf_reader->Init());
@@ -208,11 +206,11 @@ Status BloomFileReader::Open(unique_ptr<ReadableBlock> block,
 
 Status BloomFileReader::OpenNoInit(unique_ptr<ReadableBlock> block,
                                    ReaderOptions options,
-                                   gscoped_ptr<BloomFileReader> *reader) {
+                                   unique_ptr<BloomFileReader> *reader) {
   unique_ptr<CFileReader> cf_reader;
   RETURN_NOT_OK(CFileReader::OpenNoInit(std::move(block),
                                         options, &cf_reader));
-  gscoped_ptr<BloomFileReader> bf_reader(new BloomFileReader(
+  unique_ptr<BloomFileReader> bf_reader(new BloomFileReader(
       std::move(cf_reader), std::move(options)));
   if (!FLAGS_cfile_lazy_open) {
     RETURN_NOT_OK(bf_reader->Init());

http://git-wip-us.apache.org/repos/asf/kudu/blob/e2509a4b/src/kudu/cfile/bloomfile.h
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/bloomfile.h b/src/kudu/cfile/bloomfile.h
index 27c2311..6e7b280 100644
--- a/src/kudu/cfile/bloomfile.h
+++ b/src/kudu/cfile/bloomfile.h
@@ -14,8 +14,8 @@
 // KIND, either express or implied.  See the License for the
 // specific language governing permissions and limitations
 // under the License.
-#ifndef KUDU_CFILE_BLOOMFILE_H
-#define KUDU_CFILE_BLOOMFILE_H
+
+#pragma once
 
 #include <cstddef>
 #include <cstdint>
@@ -23,7 +23,6 @@
 
 #include "kudu/cfile/cfile_reader.h"
 #include "kudu/cfile/cfile_writer.h"
-#include "kudu/gutil/gscoped_ptr.h"
 #include "kudu/gutil/macros.h"
 #include "kudu/util/bloom_filter.h"
 #include "kudu/util/faststring.h"
@@ -68,7 +67,7 @@ class BloomFileWriter {
 
   Status FinishCurrentBloomBlock();
 
-  gscoped_ptr<cfile::CFileWriter> writer_;
+  std::unique_ptr<cfile::CFileWriter> writer_;
 
   BloomFilterBuilder bloom_builder_;
 
@@ -92,7 +91,7 @@ class BloomFileReader {
   // After this call, the bloom reader is safe for use.
   static Status Open(std::unique_ptr<fs::ReadableBlock> block,
                      ReaderOptions options,
-                     gscoped_ptr<BloomFileReader> *reader);
+                     std::unique_ptr<BloomFileReader>* reader);
 
   // Lazily opens a bloom file using a previously opened block. A lazy open
   // does not incur additional I/O, nor does it validate the contents of
@@ -101,7 +100,7 @@ class BloomFileReader {
   // Init() must be called before using CheckKeyPresent().
   static Status OpenNoInit(std::unique_ptr<fs::ReadableBlock> block,
                            ReaderOptions options,
-                           gscoped_ptr<BloomFileReader> *reader);
+                           std::unique_ptr<BloomFileReader>* reader);
 
   // Fully opens a previously lazily opened bloom file, parsing and
   // validating its contents.
@@ -114,7 +113,7 @@ class BloomFileReader {
   // Sets *maybe_present to false if the key is definitely not
   // present, otherwise sets it to true to indicate maybe present.
   Status CheckKeyPresent(const BloomKeyProbe &probe,
-                         bool *maybe_present);
+                         bool* maybe_present);
 
   // Can be called before Init().
   uint64_t FileSize() const {
@@ -132,8 +131,8 @@ class BloomFileReader {
   // a Slice to the true bloom filter data inside
   // *bloom_data.
   Status ParseBlockHeader(const Slice &block,
-                          BloomBlockHeaderPB *hdr,
-                          Slice *bloom_data) const;
+                          BloomBlockHeaderPB* hdr,
+                          Slice* bloom_data) const;
 
   // Callback used in 'init_once_' to initialize this bloom file.
   Status InitOnce();
@@ -158,4 +157,3 @@ class BloomFileReader {
 } // namespace cfile
 } // namespace kudu
 
-#endif

http://git-wip-us.apache.org/repos/asf/kudu/blob/e2509a4b/src/kudu/tablet/cfile_set.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/cfile_set.cc b/src/kudu/tablet/cfile_set.cc
index c1e7f10..12a9fa3 100644
--- a/src/kudu/tablet/cfile_set.cc
+++ b/src/kudu/tablet/cfile_set.cc
@@ -18,39 +18,46 @@
 #include <algorithm>
 #include <memory>
 #include <ostream>
-#include <utility>
+#include <string>
+#include <vector>
 
 #include <boost/optional/optional.hpp>
 #include <gflags/gflags.h>
 #include <glog/logging.h>
 
 #include "kudu/cfile/bloomfile.h"
+#include "kudu/cfile/cfile_reader.h"
 #include "kudu/cfile/cfile_util.h"
 #include "kudu/common/column_materialization_context.h"
 #include "kudu/common/columnblock.h"
 #include "kudu/common/encoded_key.h"
 #include "kudu/common/iterator_stats.h"
 #include "kudu/common/rowblock.h"
+#include "kudu/common/rowid.h"
 #include "kudu/common/scan_spec.h"
+#include "kudu/common/schema.h"
+#include "kudu/fs/block_id.h"
 #include "kudu/fs/block_manager.h"
 #include "kudu/fs/fs_manager.h"
 #include "kudu/gutil/dynamic_annotations.h"
-#include "kudu/gutil/map-util.h"
+#include "kudu/gutil/macros.h"
+#include "kudu/gutil/port.h"
 #include "kudu/gutil/stringprintf.h"
 #include "kudu/gutil/strings/substitute.h"
 #include "kudu/tablet/cfile_set.h"
 #include "kudu/tablet/diskrowset.h"
 #include "kudu/tablet/rowset.h"
+#include "kudu/tablet/rowset_metadata.h"
 #include "kudu/util/flag_tags.h"
 #include "kudu/util/logging.h"
 #include "kudu/util/slice.h"
+#include "kudu/util/status.h"
 
 DEFINE_bool(consult_bloom_filters, true, "Whether to consult bloom filters on row presence
checks");
 TAG_FLAG(consult_bloom_filters, hidden);
 
 namespace kudu {
 
-class BlockId;
 class MemTracker;
 
 namespace tablet {
@@ -224,10 +231,7 @@ uint64_t CFileSet::AdhocIndexOnDiskSize() const {
 }
 
 uint64_t CFileSet::BloomFileOnDiskSize() const {
-  if (bloom_reader_) {
-    return bloom_reader_->FileSize();
-  }
-  return 0;
+  return bloom_reader_->FileSize();
 }
 
 uint64_t CFileSet::OnDiskDataSize() const {
@@ -241,7 +245,7 @@ uint64_t CFileSet::OnDiskDataSize() const {
 Status CFileSet::FindRow(const RowSetKeyProbe &probe,
                          boost::optional<rowid_t>* idx,
                          ProbeStats* stats) const {
-  if (bloom_reader_ != nullptr && FLAGS_consult_bloom_filters) {
+  if (FLAGS_consult_bloom_filters) {
     // Fully open the BloomFileReader if it was lazily opened earlier.
     //
     // If it's already initialized, this is a no-op.
@@ -255,9 +259,8 @@ Status CFileSet::FindRow(const RowSetKeyProbe &probe,
       return Status::OK();
     }
     if (!s.ok()) {
-      LOG(WARNING) << "Unable to query bloom: " << s.ToString()
-                   << " (disabling bloom for this rowset from this point forward)";
-      const_cast<CFileSet *>(this)->bloom_reader_.reset(nullptr);
+      KLOG_EVERY_N_SECS(WARNING, 1) << Substitute("Unable to query bloom in $0: $1",
+          rowset_metadata_->bloom_block().ToString(), s.ToString());
       if (PREDICT_FALSE(s.IsDiskFailure())) {
         // If the bloom lookup failed because of a disk failure, return early
         // since I/O to the tablet should be stopped.
@@ -271,7 +274,7 @@ Status CFileSet::FindRow(const RowSetKeyProbe &probe,
   CFileIterator *key_iter = nullptr;
   RETURN_NOT_OK(NewKeyIterator(&key_iter));
 
-  gscoped_ptr<CFileIterator> key_iter_scoped(key_iter); // free on return
+  unique_ptr<CFileIterator> key_iter_scoped(key_iter); // free on return
 
   bool exact;
   Status s = key_iter->SeekAtOrAfter(probe.encoded_key(), &exact);

http://git-wip-us.apache.org/repos/asf/kudu/blob/e2509a4b/src/kudu/tablet/cfile_set.h
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/cfile_set.h b/src/kudu/tablet/cfile_set.h
index 0a5f409..cfd28d2 100644
--- a/src/kudu/tablet/cfile_set.h
+++ b/src/kudu/tablet/cfile_set.h
@@ -159,7 +159,7 @@ class CFileSet : public std::enable_shared_from_this<CFileSet> {
   // and is not embedded with the column's data blocks. This is used when the
   // index pertains to more than one column, as in the case of composite keys.
   std::unique_ptr<cfile::CFileReader> ad_hoc_idx_reader_;
-  gscoped_ptr<cfile::BloomFileReader> bloom_reader_;
+  std::unique_ptr<cfile::BloomFileReader> bloom_reader_;
 };
 
 


Mime
View raw message