Todd Lipcon has posted comments on this change.

Change subject: KUDU-236 (part 2). Create randomized tablet history GC itest

Patch Set 10:

File src/kudu/integration-tests/

Line 125:     ACTIONS_NUM_ITEMS = 8, // Count of items in this enum. Update if 
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, 
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 " << 
              :     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:       
              :                 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);

Line 560:         //int read_delay_rounds = 
random.Uniform(FLAGS_test_num_rounds / 10);

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I27228bac163b19334183103c77c4f818cf3f459c
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <>
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to