Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12178 )
Change subject: KUDU-2656: pass IOContext to ValidateDeltaOrder ...................................................................... Patch Set 2: (4 comments) A few nits http://gerrit.cloudera.org:8080/#/c/12178/2/src/kudu/tablet/delta_tracker.cc File src/kudu/tablet/delta_tracker.cc: http://gerrit.cloudera.org:8080/#/c/12178/2/src/kudu/tablet/delta_tracker.cc@207 PS2, Line 207: Status DeltaTracker::ValidateDeltaOrder(const std::shared_ptr<DeltaStore>& first, : const std::shared_ptr<DeltaStore>& second, : const IOContext* io_context, : DeltaType type) { : shared_ptr<DeltaStore> first_copy = first; : shared_ptr<DeltaStore> second_copy = second; : : // Make clones so we don't leave the original ones initted. That can affect : // tests. We know it's a DeltaFileReader if it's not Initted(). : if (!first_copy->Initted()) { : shared_ptr<DeltaFileReader> first_clone; : RETURN_NOT_OK(down_cast<DeltaFileReader*>(first.get())->CloneForDebugging( : rowset_metadata_->fs_manager(), mem_trackers_.tablet_tracker, &first_clone)); : RETURN_NOT_OK(first_clone->Init(io_context)); : first_copy = first_clone; : } : if (!second_copy->Initted()) { : shared_ptr<DeltaFileReader> second_clone; : RETURN_NOT_OK(down_cast<DeltaFileReader*>(second.get())->CloneForDebugging( : rowset_metadata_->fs_manager(), mem_trackers_.tablet_tracker, &second_clone)); : RETURN_NOT_OK(second_clone->Init(io_context)); : second_copy = second_clone; : } : : switch (type) { : case REDO: : DCHECK_LE(first_copy->delta_stats().min_timestamp(), : second_copy->delta_stats().min_timestamp()) : << "Found out-of-order deltas: [{" << first_copy->ToString() << "}, {" : << second_copy->ToString() << "}]: type = " << type; : break; : case UNDO: : DCHECK_GE(first_copy->delta_stats().min_timestamp(), : second_copy->delta_stats().min_timestamp()) : << "Found out-of-order deltas: [{" << first_copy->ToString() << "}, {" : << second_copy->ToString() << "}]: type = " << type; : break; : } : return Status::OK(); : } : : Status DeltaTracker::ValidateDeltasOrdered(const SharedDeltaStoreVector& list, : const IOContext* io_context, : DeltaType type) { : for (size_t i = 1; i < list.size(); i++) { : RETURN_NOT_OK(ValidateDeltaOrder(list[i - 1], list[i], io_context, type)); : } : return Status::OK(); : } If this is not used elsewhere but only in ifndef NDEBUG case, maybe put those under '#ifndef NDEBUG' as well? http://gerrit.cloudera.org:8080/#/c/12178/2/src/kudu/tablet/major_delta_compaction-test.cc File src/kudu/tablet/major_delta_compaction-test.cc: http://gerrit.cloudera.org:8080/#/c/12178/2/src/kudu/tablet/major_delta_compaction-test.cc@186 PS2, Line 186: const nit: constexpr ? http://gerrit.cloudera.org:8080/#/c/12178/2/src/kudu/tablet/major_delta_compaction-test.cc@191 PS2, Line 191: all_rowsets.front(); paranoid nit: is it guaranteed that all_rowsets container is non-empty? If not, maybe add ASSERT_FALSE(all_rowsets.empty())? http://gerrit.cloudera.org:8080/#/c/12178/2/src/kudu/tablet/major_delta_compaction-test.cc@205 PS2, Line 205: ASSERT_TRUE(s.IsCorruption()); nit: maybe, add ' << s.ToString(); for easier debugging if that fails -- To view, visit http://gerrit.cloudera.org:8080/12178 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3364ad95a5b3608db6538151007c4b6d16500f2b Gerrit-Change-Number: 12178 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 08 Jan 2019 18:05:39 +0000 Gerrit-HasComments: Yes
