You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by ad...@apache.org on 2019/12/04 17:36:37 UTC

[kudu] branch master updated: log: change return signature in several LogReader functions to void

This is an automated email from the ASF dual-hosted git repository.

adar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git


The following commit(s) were added to refs/heads/master by this push:
     new 1320073  log: change return signature in several LogReader functions to void
1320073 is described below

commit 13200733d0102018bafaada80ff1b22d858836ec
Author: Adar Dembo <ad...@cloudera.com>
AuthorDate: Tue Dec 3 21:48:24 2019 -0800

    log: change return signature in several LogReader functions to void
    
    They were previously returning a Status, but would always yield OK.
    
    Change-Id: I0afa3d2284317186101196a695c3cdd8a0fe8785
    Reviewed-on: http://gerrit.cloudera.org:8080/14826
    Reviewed-by: Alexey Serbin <as...@cloudera.com>
    Tested-by: Kudu Jenkins
---
 src/kudu/consensus/log-test.cc                     | 46 +++++++++++-----------
 src/kudu/consensus/log.cc                          | 26 ++++++------
 src/kudu/consensus/log.h                           |  4 +-
 src/kudu/consensus/log_reader.cc                   | 10 ++---
 src/kudu/consensus/log_reader.h                    |  6 +--
 src/kudu/consensus/mt-log-test.cc                  |  2 +-
 src/kudu/consensus/raft_consensus_quorum-test.cc   |  2 +-
 src/kudu/integration-tests/log_verifier.cc         |  2 +-
 .../timestamp_advancement-itest.cc                 |  2 +-
 src/kudu/tablet/tablet_bootstrap-test.cc           |  4 +-
 src/kudu/tablet/tablet_bootstrap.cc                |  2 +-
 src/kudu/tablet/tablet_replica-test.cc             | 32 +++++++--------
 src/kudu/tools/tool_action_local_replica.cc        |  4 +-
 src/kudu/tserver/tablet_copy_client-test.cc        |  2 +-
 src/kudu/tserver/tablet_copy_service-test.cc       |  2 +-
 15 files changed, 69 insertions(+), 77 deletions(-)

diff --git a/src/kudu/consensus/log-test.cc b/src/kudu/consensus/log-test.cc
index ae15fd5..8ba36f1 100644
--- a/src/kudu/consensus/log-test.cc
+++ b/src/kudu/consensus/log-test.cc
@@ -196,7 +196,7 @@ TEST_P(LogTestOptionalCompression, TestMultipleEntriesInABatch) {
   ASSERT_OK(log_->AllocateSegmentAndRollOver());
 
   SegmentSequence segments;
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
 
   LogEntries entries;
   ASSERT_OK(segments[0]->ReadEntries(&entries));
@@ -262,13 +262,13 @@ TEST_P(LogTestOptionalCompression, TestSizeIsMaintained) {
   AppendNoOp(&opid);
 
   SegmentSequence segments;
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
   int64_t orig_size = segments[0]->file_size();
   ASSERT_GT(orig_size, 0);
 
   AppendNoOp(&opid);
 
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
   int64_t new_size = segments[0]->file_size();
   ASSERT_GT(new_size, orig_size);
 
@@ -287,7 +287,7 @@ TEST_P(LogTestOptionalCompression, TestLogNotTrimmed) {
   AppendNoOp(&opid);
 
   SegmentSequence segments;
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
 
   LogEntries entries;
   ASSERT_OK(segments[0]->ReadEntries(&entries));
@@ -307,7 +307,7 @@ TEST_P(LogTestOptionalCompression, TestBlankLogFile) {
 
   // ...and we're able to read from it.
   SegmentSequence segments;
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
 
   LogEntries entries;
   ASSERT_OK(segments[0]->ReadEntries(&entries));
@@ -352,7 +352,7 @@ void LogTest::DoCorruptionTest(CorruptionType type, CorruptionPosition place,
   ASSERT_EQ(1, reader->num_segments());
 
   SegmentSequence segments;
-  ASSERT_OK(reader->GetSegmentsSnapshot(&segments));
+  reader->GetSegmentsSnapshot(&segments);
   Status s = segments[0]->ReadEntries(&entries_);
   ASSERT_EQ(s.CodeAsString(), expected_status.CodeAsString())
       << "Got unexpected status: " << s.ToString();
@@ -396,13 +396,13 @@ TEST_P(LogTestOptionalCompression, TestSegmentRollover) {
   int num_entries = 0;
 
   SegmentSequence segments;
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
 
   while (segments.size() < 3) {
     ASSERT_OK(AppendNoOps(&op_id, kNumEntriesPerBatch));
     num_entries += kNumEntriesPerBatch;
     // Update the segments
-    ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+    log_->reader()->GetSegmentsSnapshot(&segments);
   }
 
   ASSERT_FALSE(segments.back()->HasFooter());
@@ -410,7 +410,7 @@ TEST_P(LogTestOptionalCompression, TestSegmentRollover) {
 
   shared_ptr<LogReader> reader;
   ASSERT_OK(LogReader::Open(fs_manager_.get(), nullptr, kTestTablet, nullptr, &reader));
-  ASSERT_OK(reader->GetSegmentsSnapshot(&segments));
+  reader->GetSegmentsSnapshot(&segments);
 
   ASSERT_TRUE(segments.back()->HasFooter());
 
@@ -433,7 +433,7 @@ TEST_F(LogTest, TestWriteAndReadToAndFromInProgressSegment) {
   ASSERT_OK(BuildLog());
 
   SegmentSequence segments;
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(segments.size(), 1);
   scoped_refptr<ReadableLogSegment> readable_segment = segments[0];
 
@@ -494,7 +494,7 @@ TEST_F(LogTest, TestWriteAndReadToAndFromInProgressSegment) {
 
   // Now that we closed the original segment. If we get a segment from the reader
   // again, we should get one with a footer and we should be able to read all entries.
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(2, segments.size());
   readable_segment = segments[0];
   entries.clear();
@@ -529,13 +529,13 @@ TEST_P(LogTestOptionalCompression, TestGCWithLogRunning) {
   ASSERT_EQ(anchors.size(), 4);
 
   // Anchors should prevent GC.
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(4, segments.size()) << DumpSegmentsToString(segments);
   RetentionIndexes retention;
   ASSERT_OK(log_anchor_registry_->GetEarliestRegisteredLogIndex(&retention.for_durability));
   ASSERT_OK(log_->GC(retention, &num_gced_segments));
   ASSERT_EQ(0, num_gced_segments);
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(4, segments.size()) << DumpSegmentsToString(segments);
 
   // Logs should be retained for durability even if this puts it above the
@@ -567,7 +567,7 @@ TEST_P(LogTestOptionalCompression, TestGCWithLogRunning) {
   // Try again without the modified flag.
   ASSERT_OK(log_->GC(retention, &num_gced_segments));
   ASSERT_EQ(2, num_gced_segments) << DumpSegmentsToString(segments);
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(2, segments.size()) << DumpSegmentsToString(segments);
 
   // Release the remaining "rolled segment" anchor. GC will not delete the
@@ -576,7 +576,7 @@ TEST_P(LogTestOptionalCompression, TestGCWithLogRunning) {
   ASSERT_OK(log_anchor_registry_->GetEarliestRegisteredLogIndex(&retention.for_durability));
   ASSERT_OK(log_->GC(retention, &num_gced_segments));
   ASSERT_EQ(0, num_gced_segments) << DumpSegmentsToString(segments);
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(2, segments.size()) << DumpSegmentsToString(segments);
 
   // Check that we get a NotFound if we try to read before the GCed point.
@@ -650,7 +650,7 @@ TEST_P(LogTestOptionalCompression, TestWaitUntilAllFlushed) {
 
   // Make sure we only get 4 entries back and that no FLUSH_MARKER commit is found.
   vector<scoped_refptr<ReadableLogSegment> > segments;
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
 
   ASSERT_OK(segments[0]->ReadEntries(&entries_));
   ASSERT_EQ(4, entries_.size());
@@ -680,12 +680,12 @@ TEST_P(LogTestOptionalCompression, TestLogReopenAndGC) {
   ASSERT_OK(AppendMultiSegmentSequence(kNumTotalSegments, kNumOpsPerSegment,
                                               &op_id, &anchors));
   // Anchors should prevent GC.
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(3, segments.size());
   RetentionIndexes retention;
   ASSERT_OK(log_anchor_registry_->GetEarliestRegisteredLogIndex(&retention.for_durability));
   ASSERT_OK(log_->GC(retention, &num_gced_segments));
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(3, segments.size());
 
   ASSERT_OK(log_->Close());
@@ -695,7 +695,7 @@ TEST_P(LogTestOptionalCompression, TestLogReopenAndGC) {
   ASSERT_OK(BuildLog());
 
   // The "old" data consists of 3 segments. We still hold anchors.
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(4, segments.size());
 
   // Write to a new log segment, as if we had taken new requests and the
@@ -725,7 +725,7 @@ TEST_P(LogTestOptionalCompression, TestLogReopenAndGC) {
 
   // After GC there should be only one left, besides the one currently being
   // written to. That is because min_segments_to_retain defaults to 2.
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(2, segments.size()) << DumpSegmentsToString(segments);
   ASSERT_OK(log_->Close());
 
@@ -758,7 +758,7 @@ TEST_P(LogTestOptionalCompression, TestWriteManyBatches) {
 
     shared_ptr<LogReader> reader;
     ASSERT_OK(LogReader::Open(fs_manager_.get(), nullptr, kTestTablet, nullptr, &reader));
-    ASSERT_OK(reader->GetSegmentsSnapshot(&segments));
+    reader->GetSegmentsSnapshot(&segments);
 
     for (const scoped_refptr<ReadableLogSegment>& entry : segments) {
       entries_.clear();
@@ -814,7 +814,7 @@ TEST_P(LogTestOptionalCompression, TestLogReaderReturnsLatestSegmentIfIndexEmpty
   ASSERT_OK(AppendReplicateBatch(opid, APPEND_SYNC));
 
   SegmentSequence segments;
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(segments.size(), 1);
 
   LogEntries entries;
@@ -1185,7 +1185,7 @@ TEST_P(LogTestOptionalCompression, TestTotalSize) {
   int num_gced_segments;
   ASSERT_OK(log_->GC(retention, &num_gced_segments));
   ASSERT_EQ(1, num_gced_segments) << DumpSegmentsToString(segments);
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(2, segments.size()) << DumpSegmentsToString(segments);
 
   // Now we've added two segments and GC'd one, so the total size should be
diff --git a/src/kudu/consensus/log.cc b/src/kudu/consensus/log.cc
index 5b19017..6665b38 100644
--- a/src/kudu/consensus/log.cc
+++ b/src/kudu/consensus/log.cc
@@ -801,7 +801,7 @@ Status Log::Init() {
                         << " segments from path: " << ctx_.fs_manager->GetWalsRootDir();
 
     vector<scoped_refptr<ReadableLogSegment> > segments;
-    RETURN_NOT_OK(reader_->GetSegmentsSnapshot(&segments));
+    reader_->GetSegmentsSnapshot(&segments);
     active_seg_seq_num = segments.back()->header().sequence_number();
   }
 
@@ -968,11 +968,10 @@ int GetPrefixSizeToGC(RetentionIndexes retention_indexes, const SegmentSequence&
   return prefix_size;
 }
 
-Status Log::GetSegmentsToGCUnlocked(RetentionIndexes retention_indexes,
+void Log::GetSegmentsToGCUnlocked(RetentionIndexes retention_indexes,
                                     SegmentSequence* segments_to_gc) const {
-  RETURN_NOT_OK(reader_->GetSegmentsSnapshot(segments_to_gc));
+  reader_->GetSegmentsSnapshot(segments_to_gc);
   segments_to_gc->resize(GetPrefixSizeToGC(retention_indexes, *segments_to_gc));
-  return Status::OK();
 }
 
 Status Log::Append(LogEntryPB* entry) {
@@ -1013,7 +1012,7 @@ Status Log::GC(RetentionIndexes retention_indexes, int32_t* num_gced) {
       std::lock_guard<percpu_rwlock> l(state_lock_);
       CHECK_EQ(kLogWriting, log_state_);
 
-      RETURN_NOT_OK(GetSegmentsToGCUnlocked(retention_indexes, &segments_to_delete));
+      GetSegmentsToGCUnlocked(retention_indexes, &segments_to_delete);
 
       if (segments_to_delete.empty()) {
         VLOG_WITH_PREFIX(1) << "No segments to delete.";
@@ -1022,8 +1021,8 @@ Status Log::GC(RetentionIndexes retention_indexes, int32_t* num_gced) {
       }
       // Trim the prefix of segments from the reader so that they are no longer
       // referenced by the log.
-      RETURN_NOT_OK(reader_->TrimSegmentsUpToAndIncluding(
-          segments_to_delete[segments_to_delete.size() - 1]->header().sequence_number()));
+      reader_->TrimSegmentsUpToAndIncluding(
+          segments_to_delete[segments_to_delete.size() - 1]->header().sequence_number());
     }
 
     // Now that they are no longer referenced by the Log, delete the files.
@@ -1057,14 +1056,10 @@ int64_t Log::GetGCableDataSize(RetentionIndexes retention_indexes) const {
   {
     shared_lock<rw_spinlock> l(state_lock_.get_lock());
     CHECK_EQ(kLogWriting, log_state_);
-    Status s = GetSegmentsToGCUnlocked(retention_indexes, &segments_to_delete);
-
-    if (!s.ok() || segments_to_delete.empty()) {
-      return 0;
-    }
+    GetSegmentsToGCUnlocked(retention_indexes, &segments_to_delete);
   }
   int64_t total_size = 0;
-  for (const scoped_refptr<ReadableLogSegment>& segment : segments_to_delete) {
+  for (const auto& segment : segments_to_delete) {
     total_size += segment->file_size();
   }
   return total_size;
@@ -1076,7 +1071,7 @@ void Log::GetReplaySizeMap(std::map<int64_t, int64_t>* replay_size) const {
   {
     shared_lock<rw_spinlock> l(state_lock_.get_lock());
     CHECK_EQ(kLogWriting, log_state_);
-    CHECK_OK(reader_->GetSegmentsSnapshot(&segments));
+    reader_->GetSegmentsSnapshot(&segments);
   }
 
   int64_t cumulative_size = 0;
@@ -1094,9 +1089,10 @@ int64_t Log::OnDiskSize() {
     shared_lock<rw_spinlock> l(state_lock_.get_lock());
     // If the log is closed, the tablet is either being deleted or tombstoned,
     // so we don't count the size of its log anymore as it should be deleted.
-    if (log_state_ == kLogClosed || !reader_->GetSegmentsSnapshot(&segments).ok()) {
+    if (log_state_ == kLogClosed) {
       return on_disk_size_.load();
     }
+    reader_->GetSegmentsSnapshot(&segments);
   }
   int64_t ret = 0;
   for (const auto& segment : segments) {
diff --git a/src/kudu/consensus/log.h b/src/kudu/consensus/log.h
index 66e43d8..b5e6e3d 100644
--- a/src/kudu/consensus/log.h
+++ b/src/kudu/consensus/log.h
@@ -465,8 +465,8 @@ class Log : public RefCountedThreadSafe<Log> {
   Status Sync();
 
   // Helper method to get the segment sequence to GC based on the provided 'retention' struct.
-  Status GetSegmentsToGCUnlocked(RetentionIndexes retention_indexes,
-                                 SegmentSequence* segments_to_gc) const;
+  void GetSegmentsToGCUnlocked(RetentionIndexes retention_indexes,
+                               SegmentSequence* segments_to_gc) const;
 
   LogEntryBatchQueue* entry_queue() {
     return &entry_batch_queue_;
diff --git a/src/kudu/consensus/log_reader.cc b/src/kudu/consensus/log_reader.cc
index f3ae935..662af85 100644
--- a/src/kudu/consensus/log_reader.cc
+++ b/src/kudu/consensus/log_reader.cc
@@ -198,13 +198,11 @@ Status LogReader::Init(const string& tablet_wal_path) {
   return Status::OK();
 }
 
-Status LogReader::InitEmptyReaderForTests() {
+void LogReader::InitEmptyReaderForTests() {
   std::lock_guard<simple_spinlock> lock(lock_);
   state_ = kLogReaderReading;
-  return Status::OK();
 }
 
-
 int64_t LogReader::GetMinReplicateIndex() const {
   std::lock_guard<simple_spinlock> lock(lock_);
   int64_t min_remaining_op_idx = -1;
@@ -357,14 +355,13 @@ Status LogReader::LookupOpId(int64_t op_index, OpId* op_id) const {
   return Status::OK();
 }
 
-Status LogReader::GetSegmentsSnapshot(SegmentSequence* segments) const {
+void LogReader::GetSegmentsSnapshot(SegmentSequence* segments) const {
   std::lock_guard<simple_spinlock> lock(lock_);
   CHECK_EQ(state_, kLogReaderReading);
   segments->assign(segments_.begin(), segments_.end());
-  return Status::OK();
 }
 
-Status LogReader::TrimSegmentsUpToAndIncluding(int64_t segment_sequence_number) {
+void LogReader::TrimSegmentsUpToAndIncluding(int64_t segment_sequence_number) {
   std::lock_guard<simple_spinlock> lock(lock_);
   CHECK_EQ(state_, kLogReaderReading);
   auto iter = segments_.begin();
@@ -380,7 +377,6 @@ Status LogReader::TrimSegmentsUpToAndIncluding(int64_t segment_sequence_number)
   }
   LOG(INFO) << "T " << tablet_id_ << ": removed " << num_deleted_segments
             << " log segments from log reader";
-  return Status::OK();
 }
 
 void LogReader::UpdateLastSegmentOffset(int64_t readable_to_offset) {
diff --git a/src/kudu/consensus/log_reader.h b/src/kudu/consensus/log_reader.h
index cbf4c2d..3325233 100644
--- a/src/kudu/consensus/log_reader.h
+++ b/src/kudu/consensus/log_reader.h
@@ -87,7 +87,7 @@ class LogReader : public enable_make_shared<LogReader> {
 
   // Copies a snapshot of the current sequence of segments into 'segments'.
   // 'segments' will be cleared first.
-  Status GetSegmentsSnapshot(SegmentSequence* segments) const;
+  void GetSegmentsSnapshot(SegmentSequence* segments) const;
 
   // Reads all ReplicateMsgs from 'starting_at' to 'up_to' both inclusive.
   // The caller takes ownership of the returned ReplicateMsg objects.
@@ -142,7 +142,7 @@ class LogReader : public enable_make_shared<LogReader> {
 
   // Removes segments with sequence numbers less than or equal to
   // 'segment_sequence_number' from this reader.
-  Status TrimSegmentsUpToAndIncluding(int64_t segment_sequence_number);
+  void TrimSegmentsUpToAndIncluding(int64_t segment_sequence_number);
 
   // Replaces the last segment in the reader with 'segment'.
   // Used to replace a segment that was still in the process of being written
@@ -175,7 +175,7 @@ class LogReader : public enable_make_shared<LogReader> {
   Status Init(const std::string& tablet_wal_path);
 
   // Initializes an 'empty' reader for tests, i.e. does not scan a path looking for segments.
-  Status InitEmptyReaderForTests();
+  void InitEmptyReaderForTests();
 
   Env* env_;
   const scoped_refptr<LogIndex> log_index_;
diff --git a/src/kudu/consensus/mt-log-test.cc b/src/kudu/consensus/mt-log-test.cc
index d4ced17..cb9329f 100644
--- a/src/kudu/consensus/mt-log-test.cc
+++ b/src/kudu/consensus/mt-log-test.cc
@@ -206,7 +206,7 @@ class MultiThreadedLogTest : public LogTestBase {
     shared_ptr<LogReader> reader;
     ASSERT_OK(LogReader::Open(fs_manager_.get(), nullptr, kTestTablet, nullptr, &reader));
     SegmentSequence segments;
-    ASSERT_OK(reader->GetSegmentsSnapshot(&segments));
+    reader->GetSegmentsSnapshot(&segments);
 
     for (const SegmentSequence::value_type& entry : segments) {
       ASSERT_OK(entry->ReadEntries(&entries_));
diff --git a/src/kudu/consensus/raft_consensus_quorum-test.cc b/src/kudu/consensus/raft_consensus_quorum-test.cc
index abcdf5c..9d4209e 100644
--- a/src/kudu/consensus/raft_consensus_quorum-test.cc
+++ b/src/kudu/consensus/raft_consensus_quorum-test.cc
@@ -436,7 +436,7 @@ class RaftConsensusQuorumTest : public KuduTest {
                                    metric_entity_.get(),
                                    &log_reader));
     log::SegmentSequence segments;
-    ASSERT_OK(log_reader->GetSegmentsSnapshot(&segments));
+    log_reader->GetSegmentsSnapshot(&segments);
 
     LogEntries ret;
     for (const log::SegmentSequence::value_type& entry : segments) {
diff --git a/src/kudu/integration-tests/log_verifier.cc b/src/kudu/integration-tests/log_verifier.cc
index 280b8fe..48be779 100644
--- a/src/kudu/integration-tests/log_verifier.cc
+++ b/src/kudu/integration-tests/log_verifier.cc
@@ -78,7 +78,7 @@ Status LogVerifier::ScanForCommittedOpIds(int ts_idx, const string& tablet_id,
   RETURN_NOT_OK(LogReader::Open(env_, wal_dir, scoped_refptr<log::LogIndex>(), tablet_id,
                                 scoped_refptr<MetricEntity>(), &reader));
   log::SegmentSequence segs;
-  RETURN_NOT_OK(reader->GetSegmentsSnapshot(&segs));
+  reader->GetSegmentsSnapshot(&segs);
   unique_ptr<log::LogEntryPB> entry;
   for (const auto& seg : segs) {
     log::LogEntryReader reader(seg.get());
diff --git a/src/kudu/integration-tests/timestamp_advancement-itest.cc b/src/kudu/integration-tests/timestamp_advancement-itest.cc
index 3e6b77d..1cfccbe 100644
--- a/src/kudu/integration-tests/timestamp_advancement-itest.cc
+++ b/src/kudu/integration-tests/timestamp_advancement-itest.cc
@@ -184,7 +184,7 @@ class TimestampAdvancementITest : public MiniClusterITestBase {
        scoped_refptr<log::LogIndex>(), tablet_id,
        scoped_refptr<MetricEntity>(), &reader));
     log::SegmentSequence segs;
-    RETURN_NOT_OK(reader->GetSegmentsSnapshot(&segs));
+    reader->GetSegmentsSnapshot(&segs);
     unique_ptr<log::LogEntryPB> entry;
     for (const auto& seg : segs) {
       log::LogEntryReader reader(seg.get());
diff --git a/src/kudu/tablet/tablet_bootstrap-test.cc b/src/kudu/tablet/tablet_bootstrap-test.cc
index ceb7d5a..8c72817 100644
--- a/src/kudu/tablet/tablet_bootstrap-test.cc
+++ b/src/kudu/tablet/tablet_bootstrap-test.cc
@@ -324,7 +324,7 @@ TEST_F(BootstrapTest, TestOrphanCommit) {
     // commits.
     ASSERT_OK(AppendCommit(opid));
     log::SegmentSequence segments;
-    ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+    log_->reader()->GetSegmentsSnapshot(&segments);
     fs_manager_->env()->DeleteFile(segments[0]->path());
 
     // Untrack the tablet in the data dir manager so upon the next call to
@@ -392,7 +392,7 @@ TEST_F(BootstrapTest, TestNonOrphansAfterOrphanCommit) {
   ASSERT_OK(AppendCommit(opid));
 
   log::SegmentSequence segments;
-  ASSERT_OK(log_->reader()->GetSegmentsSnapshot(&segments));
+  log_->reader()->GetSegmentsSnapshot(&segments);
   fs_manager_->env()->DeleteFile(segments[0]->path());
 
   current_index_ += 2;
diff --git a/src/kudu/tablet/tablet_bootstrap.cc b/src/kudu/tablet/tablet_bootstrap.cc
index 8cf55ea..dd10f3b 100644
--- a/src/kudu/tablet/tablet_bootstrap.cc
+++ b/src/kudu/tablet/tablet_bootstrap.cc
@@ -1154,7 +1154,7 @@ Status TabletBootstrap::PlaySegments(const IOContext* io_context,
                                      ConsensusBootstrapInfo* consensus_info) {
   ReplayState state;
   log::SegmentSequence segments;
-  RETURN_NOT_OK(log_reader_->GetSegmentsSnapshot(&segments));
+  log_reader_->GetSegmentsSnapshot(&segments);
 
   // The first thing to do is to rewind the tablet's schema back to the schema
   // as of the point in time where the logs begin. We must replay the writes
diff --git a/src/kudu/tablet/tablet_replica-test.cc b/src/kudu/tablet/tablet_replica-test.cc
index d1c6bcc..d7f3734 100644
--- a/src/kudu/tablet/tablet_replica-test.cc
+++ b/src/kudu/tablet/tablet_replica-test.cc
@@ -435,11 +435,11 @@ TEST_F(TabletReplicaTest, TestMRSAnchorPreventsLogGC) {
   ASSERT_EVENTUALLY([&]{ AssertNoLogAnchors(); });
 
   log::SegmentSequence segments;
-  ASSERT_OK(log->reader()->GetSegmentsSnapshot(&segments));
+  log->reader()->GetSegmentsSnapshot(&segments);
 
   ASSERT_EQ(1, segments.size());
   ASSERT_OK(ExecuteInsertsAndRollLogs(3));
-  ASSERT_OK(log->reader()->GetSegmentsSnapshot(&segments));
+  log->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(4, segments.size());
 
   NO_FATALS(AssertLogAnchorEarlierThanLogLatest());
@@ -460,7 +460,7 @@ TEST_F(TabletReplicaTest, TestMRSAnchorPreventsLogGC) {
   retention = tablet_replica_->GetRetentionIndexes();
   ASSERT_OK(log->GC(retention, &num_gced));
   ASSERT_EQ(2, num_gced) << "earliest needed: " << retention.for_durability;
-  ASSERT_OK(log->reader()->GetSegmentsSnapshot(&segments));
+  log->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(2, segments.size());
 }
 
@@ -476,11 +476,11 @@ TEST_F(TabletReplicaTest, TestDMSAnchorPreventsLogGC) {
   ASSERT_EVENTUALLY([&]{ AssertNoLogAnchors(); });
 
   log::SegmentSequence segments;
-  ASSERT_OK(log->reader()->GetSegmentsSnapshot(&segments));
+  log->reader()->GetSegmentsSnapshot(&segments);
 
   ASSERT_EQ(1, segments.size());
   ASSERT_OK(ExecuteInsertsAndRollLogs(2));
-  ASSERT_OK(log->reader()->GetSegmentsSnapshot(&segments));
+  log->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(3, segments.size());
 
   // Flush MRS & GC log so the next mutation goes into a DMS.
@@ -491,7 +491,7 @@ TEST_F(TabletReplicaTest, TestDMSAnchorPreventsLogGC) {
   // We will only GC 1, and have 1 left because the earliest needed OpId falls
   // back to the latest OpId written to the Log if no anchors are set.
   ASSERT_EQ(1, num_gced);
-  ASSERT_OK(log->reader()->GetSegmentsSnapshot(&segments));
+  log->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(2, segments.size());
 
   boost::optional<OpId> id = consensus->GetLastOpId(consensus::RECEIVED_OPID);
@@ -510,13 +510,13 @@ TEST_F(TabletReplicaTest, TestDMSAnchorPreventsLogGC) {
   ASSERT_OK(ExecuteDeletesAndRollLogs(2));
   NO_FATALS(AssertLogAnchorEarlierThanLogLatest());
   ASSERT_GT(tablet_replica_->log_anchor_registry()->GetAnchorCountForTests(), 0);
-  ASSERT_OK(log->reader()->GetSegmentsSnapshot(&segments));
+  log->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(4, segments.size());
 
   // Execute another couple inserts, but Flush it so it doesn't anchor.
   ASSERT_OK(ExecuteInsertsAndRollLogs(2));
   ASSERT_OK(tablet_replica_->tablet()->Flush());
-  ASSERT_OK(log->reader()->GetSegmentsSnapshot(&segments));
+  log->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(6, segments.size());
 
   // Ensure the delta and last insert remain in the logs, anchored by the delta.
@@ -524,7 +524,7 @@ TEST_F(TabletReplicaTest, TestDMSAnchorPreventsLogGC) {
   retention = tablet_replica_->GetRetentionIndexes();
   ASSERT_OK(log->GC(retention, &num_gced));
   ASSERT_EQ(1, num_gced);
-  ASSERT_OK(log->reader()->GetSegmentsSnapshot(&segments));
+  log->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(5, segments.size());
 
   // Flush DMS to release the anchor.
@@ -540,7 +540,7 @@ TEST_F(TabletReplicaTest, TestDMSAnchorPreventsLogGC) {
   retention = tablet_replica_->GetRetentionIndexes();
   ASSERT_OK(log->GC(retention, &num_gced));
   ASSERT_EQ(3, num_gced);
-  ASSERT_OK(log->reader()->GetSegmentsSnapshot(&segments));
+  log->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(2, segments.size());
 }
 
@@ -555,11 +555,11 @@ TEST_F(TabletReplicaTest, TestActiveTransactionPreventsLogGC) {
   ASSERT_EVENTUALLY([&]{ AssertNoLogAnchors(); });
 
   log::SegmentSequence segments;
-  ASSERT_OK(log->reader()->GetSegmentsSnapshot(&segments));
+  log->reader()->GetSegmentsSnapshot(&segments);
 
   ASSERT_EQ(1, segments.size());
   ASSERT_OK(ExecuteInsertsAndRollLogs(4));
-  ASSERT_OK(log->reader()->GetSegmentsSnapshot(&segments));
+  log->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(5, segments.size());
 
   // Flush MRS as needed to ensure that we don't have OpId anchors in the MRS.
@@ -615,13 +615,13 @@ TEST_F(TabletReplicaTest, TestActiveTransactionPreventsLogGC) {
   log::RetentionIndexes retention = tablet_replica_->GetRetentionIndexes();
   ASSERT_OK(log->GC(retention, &num_gced));
   ASSERT_EQ(4, num_gced);
-  ASSERT_OK(log->reader()->GetSegmentsSnapshot(&segments));
+  log->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(2, segments.size());
 
   // We use mutations here, since an MRS Flush() quiesces the tablet, and we
   // want to ensure the only thing "anchoring" is the TransactionTracker.
   ASSERT_OK(ExecuteDeletesAndRollLogs(3));
-  ASSERT_OK(log->reader()->GetSegmentsSnapshot(&segments));
+  log->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(5, segments.size());
   ASSERT_EQ(1, tablet_replica_->log_anchor_registry()->GetAnchorCountForTests());
   tablet_replica_->tablet()->FlushBiggestDMS();
@@ -637,7 +637,7 @@ TEST_F(TabletReplicaTest, TestActiveTransactionPreventsLogGC) {
   retention = tablet_replica_->GetRetentionIndexes();
   ASSERT_OK(log->GC(retention, &num_gced));
   ASSERT_EQ(0, num_gced);
-  ASSERT_OK(log->reader()->GetSegmentsSnapshot(&segments));
+  log->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(5, segments.size());
 
   // Now we release the transaction and wait for everything to complete.
@@ -654,7 +654,7 @@ TEST_F(TabletReplicaTest, TestActiveTransactionPreventsLogGC) {
   retention = tablet_replica_->GetRetentionIndexes();
   ASSERT_OK(log->GC(retention, &num_gced));
   ASSERT_EQ(3, num_gced);
-  ASSERT_OK(log->reader()->GetSegmentsSnapshot(&segments));
+  log->reader()->GetSegmentsSnapshot(&segments);
   ASSERT_EQ(2, segments.size());
 }
 
diff --git a/src/kudu/tools/tool_action_local_replica.cc b/src/kudu/tools/tool_action_local_replica.cc
index 038baed..97d9312 100644
--- a/src/kudu/tools/tool_action_local_replica.cc
+++ b/src/kudu/tools/tool_action_local_replica.cc
@@ -188,7 +188,7 @@ Status FindLastLoggedOpId(FsManager* fs, const string& tablet_id,
   RETURN_NOT_OK(LogReader::Open(fs, scoped_refptr<log::LogIndex>(), tablet_id,
                                 scoped_refptr<MetricEntity>(), &reader));
   SegmentSequence segs;
-  RETURN_NOT_OK(reader->GetSegmentsSnapshot(&segs));
+  reader->GetSegmentsSnapshot(&segs);
   // Reverse iterate the segments to find the 'last replicated' entry quickly.
   // Note that we still read the entries within a segment in sequential
   // fashion, so the last entry within the first 'found' segment will
@@ -526,7 +526,7 @@ Status DumpWals(const RunnerContext& context) {
                                 &reader));
 
   SegmentSequence segments;
-  RETURN_NOT_OK(reader->GetSegmentsSnapshot(&segments));
+  reader->GetSegmentsSnapshot(&segments);
 
   for (const scoped_refptr<ReadableLogSegment>& segment : segments) {
     RETURN_NOT_OK(PrintSegment(segment));
diff --git a/src/kudu/tserver/tablet_copy_client-test.cc b/src/kudu/tserver/tablet_copy_client-test.cc
index c24adb6..a12782c 100644
--- a/src/kudu/tserver/tablet_copy_client-test.cc
+++ b/src/kudu/tserver/tablet_copy_client-test.cc
@@ -304,7 +304,7 @@ TEST_F(TabletCopyClientTest, TestDownloadWalSegment) {
   ASSERT_TRUE(fs_manager_->Exists(path));
 
   log::SegmentSequence local_segments;
-  ASSERT_OK(tablet_replica_->log()->reader()->GetSegmentsSnapshot(&local_segments));
+  tablet_replica_->log()->reader()->GetSegmentsSnapshot(&local_segments);
   const scoped_refptr<log::ReadableLogSegment>& segment = local_segments[0];
   string server_path = segment->path();
 
diff --git a/src/kudu/tserver/tablet_copy_service-test.cc b/src/kudu/tserver/tablet_copy_service-test.cc
index 8707f9e..2658d9f 100644
--- a/src/kudu/tserver/tablet_copy_service-test.cc
+++ b/src/kudu/tserver/tablet_copy_service-test.cc
@@ -471,7 +471,7 @@ TEST_F(TabletCopyServiceTest, TestFetchLog) {
 
   // Fetch the local data.
   log::SegmentSequence local_segments;
-  ASSERT_OK(tablet_replica_->log()->reader()->GetSegmentsSnapshot(&local_segments));
+  tablet_replica_->log()->reader()->GetSegmentsSnapshot(&local_segments);
 
   uint64_t first_seg_seqno = (*local_segments.begin())->header().sequence_number();