kudu git commit: cfile_set: make FindRow more robust to errors

[ad...@apache.org]

Repository: kudu
Updated Branches:
  refs/heads/master 354f4c445 -> edae025c4


cfile_set: make FindRow more robust to errors

Previously, FindRow() would not handle certain error cases well. I.e.
when seeking on a key, it wouldn't consider all errors before returning
the seek results.

For example, currently, FindRow() scans over CFiles and if 1) it hits a
Status::NotFound() error, or 2) the key search returned without being
an exact match, it returns success, returning the row is not present.

However, 2) does not take into account the OK-ness of the returned
scans, and thus may return healthily saying it couldn't find the row,
even though this may not be the case. This allows the currently-running
operation to continue under the potentially incorrect assumption that
the key is not present, when it should actually return with the
appropriate error.

In the case of DuplicatingRowSet::MutateRow(), this is particularly
nasty: it expects operations to be mirrored to two separate rowsets, and
the above scenario may present itself as a fatal failure to do so.

In practice, this hasn't been an issue because we might only expect
various fs-level errors (e.g. corruption, disk errors) here, which we
already don't handle gracefully. However, this needs to change to be
robust to disk failures.

Similarly, FindRow() will now return early if it hits a disk failure in
the bloom look-up.

A small test is added to diskrowset-test to ensure errors are returned
appropriately.

Change-Id: I41b56cbbf1b3b00fdb4dbf1ec08f12ced97088e7
Reviewed-on: http://gerrit.cloudera.org:8080/8423
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>


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

Branch: refs/heads/master
Commit: edae025c450e100bef2b21bc8ed0dde13ed838aa
Parents: 354f4c4
Author: Andrew Wong <aw...@cloudera.com>
Authored: Thu Oct 26 12:00:00 2017 -0700
Committer: Adar Dembo <ad...@cloudera.com>
Committed: Tue Oct 31 22:43:19 2017 +0000

----------------------------------------------------------------------
 src/kudu/tablet/cfile_set.cc       |  7 ++++++-
 src/kudu/tablet/diskrowset-test.cc | 31 +++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/edae025c/src/kudu/tablet/cfile_set.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/cfile_set.cc b/src/kudu/tablet/cfile_set.cc
index ede09d4..c1e7f10 100644
--- a/src/kudu/tablet/cfile_set.cc
+++ b/src/kudu/tablet/cfile_set.cc
@@ -258,6 +258,11 @@ Status CFileSet::FindRow(const RowSetKeyProbe &probe,
       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);
+      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.
+        return s;
+      }
       // Continue with the slow path
     }
   }
@@ -270,7 +275,7 @@ Status CFileSet::FindRow(const RowSetKeyProbe &probe,
 
   bool exact;
   Status s = key_iter->SeekAtOrAfter(probe.encoded_key(), &exact);
-  if (s.IsNotFound() || !exact) {
+  if (s.IsNotFound() || (s.ok() && !exact)) {
     *idx = boost::none;
     return Status::OK();
   }

http://git-wip-us.apache.org/repos/asf/kudu/blob/edae025c/src/kudu/tablet/diskrowset-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/diskrowset-test.cc b/src/kudu/tablet/diskrowset-test.cc
index 1013b3d..3005512 100644
--- a/src/kudu/tablet/diskrowset-test.cc
+++ b/src/kudu/tablet/diskrowset-test.cc
@@ -64,7 +64,9 @@
 
 DEFINE_double(update_fraction, 0.1f, "fraction of rows to update");
 DECLARE_bool(cfile_lazy_open);
+DECLARE_bool(crash_on_eio);
 DECLARE_int32(cfile_default_block_size);
+DECLARE_double(env_inject_eio);
 DECLARE_double(tablet_delta_store_major_compact_min_ratio);
 DECLARE_int32(tablet_delta_store_minor_compact_max);
 
@@ -196,6 +198,35 @@ TEST_F(TestRowSet, TestRowSetUpdate) {
   VerifyUpdates(*rs, updated);
 }
 
+TEST_F(TestRowSet, TestErrorDuringUpdate) {
+  WriteTestRowSet();
+  shared_ptr<DiskRowSet> rs;
+  ASSERT_OK(OpenTestRowSet(&rs));
+
+  faststring buf;
+  RowChangeListEncoder enc(&buf);
+  enc.SetToDelete();
+
+  // Get a row that we expect to be in the rowset.
+  Timestamp timestamp(0);
+  RowBuilder rb(schema_.CreateKeyProjection());
+  rb.AddString(Slice("hello 000000000000050"));
+  RowSetKeyProbe probe(rb.row());
+
+  // But fail while reading it!
+  FLAGS_crash_on_eio = false;
+  FLAGS_env_inject_eio = 1.0;
+
+  // The mutation should result in an IOError.
+  OperationResultPB result;
+  ProbeStats stats;
+  Status s = rs->MutateRow(timestamp, probe, enc.as_changelist(), op_id_, &stats, &result);
+  LOG(INFO) << s.ToString();
+  ASSERT_TRUE(s.IsIOError());
+
+  FLAGS_env_inject_eio = 0;
+}
+
 TEST_F(TestRowSet, TestRandomRead) {
   // Write 100 rows.
   WriteTestRowSet(100);