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 <mpe...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to