Mike Percy 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 Done Line 127: static const int kNumActions = ACTIONS_NUM_ITEMS; > why not just use kNumActions as the enum value on line 125? Done Line 130: using MaterializedTestTable = std::map<int32_t, MaterializedTestRow>; > oh shit! c++11 'using' declarations! I award thee 3 Burkert Points for usag :D 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 th Thanks, that's exactly this logic. Line 186: scanners_.insert(std::move(entry)); > could you do this a little more succintly with: I tried that when writing this. Just tried a bunch of things again and I couldn't get them to work... Having a tough time figuring out exactly why... I just get template mismatch errors. 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 typede Done Line 196: scanners_.erase(old_iter); > somewhat surprising that this function actually removes the iterator elemen Did the 2nd thing PS10, Line 214: if (snap == nullptr) { : FAIL() << "Could not find snapshot to match timestamp " << snap_ts.ToString(); : } > how about just ASSERT_NOTNULL(snap) << ... used ASSERT_NE since ASSERT_NOTNULL has a return value PS10, Line 423: (random.OneIn(20) == 0); > I think this condition is wrong -- OneIn(20) is already a boolean true 5% o Thanks for the catch PS10, Line 482: (random.OneIn(20) == 0); > same Done Line 560: //int read_delay_rounds = random.Uniform(FLAGS_test_num_rounds / 10); > remove Done -- 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
