Todd Lipcon has posted comments on this change. Change subject: KUDU-236 (part 2). Create randomized tablet history GC itest ......................................................................
Patch Set 10: (11 comments) http://gerrit.cloudera.org:8080/#/c/3975/10/src/kudu/integration-tests/tablet_history_gc-itest.cc File src/kudu/integration-tests/tablet_history_gc-itest.cc: Line 125: ACTIONS_NUM_ITEMS = 8, // Count of items in this enum. Update if needed. why assign the integers here explicitly? the enum is already defined to go from 0 to the number of elements Line 127: static const int kNumActions = ACTIONS_NUM_ITEMS; why not just use kNumActions as the enum value on line 125? Line 130: using MaterializedTestTable = std::map<int32_t, MaterializedTestRow>; oh shit! c++11 'using' declarations! I award thee 3 Burkert Points for usage of new features. PS10, Line 156: auto iter = snapshots_.upper_bound(ts.ToUint64()); : if (iter == snapshots_.begin()) { : LOG(FATAL) << "There is no saved snapshot with a TS <= " << ts.ToUint64(); : } : --iter; : VLOG(3) << "Using verification snapshot from TS " << StringifyTimestamp(Timestamp(iter->first)); : return &iter->second; : can you use FindFloorOrDie here? (or FindFloorOrNull if you want to keep the nicer fatal message) Line 186: scanners_.insert(std::move(entry)); could you do this a little more succintly with: scanners_.emplace(verify_round, {std::move(scanner), snap_ts}); PS10, Line 192: NO_FATALS(VerifySnapshotScan(std::move(iter->second.first), : std::move(iter->second.second), iter->first)); : all these firsts/seconds are a bit confusing (especially because the typedefs mean the type parameters aren't visible anywhere near here). Mind unpacking them into local variables with names first? Line 196: scanners_.erase(old_iter); somewhat surprising that this function actually removes the iterator elements. Maybe make this pure iteration, and then just use scanners_.erase(...) at the call site to erase the range? (or clear in the case of AllRemaining) alternatively, rename this to VerifyAndRemoveScanners or something? PS10, Line 214: if (snap == nullptr) { : FAIL() << "Could not find snapshot to match timestamp " << snap_ts.ToString(); : } how about just ASSERT_NOTNULL(snap) << ... PS10, Line 423: (random.OneIn(20) == 0); I think this condition is wrong -- OneIn(20) is already a boolean true 5% of the time PS10, Line 482: (random.OneIn(20) == 0); same Line 560: //int read_delay_rounds = random.Uniform(FLAGS_test_num_rounds / 10); remove -- To view, visit http://gerrit.cloudera.org:8080/3975 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I27228bac163b19334183103c77c4f818cf3f459c Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
