Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11140 )
Change subject: deltas: fuzz tests for delta file and dms ...................................................................... Patch Set 15: (8 comments) looks good overall http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h File src/kudu/tablet/tablet-test-util.h: http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h@354 PS15, Line 354: T::kTag == REDO, : std::less<Timestamp>, : std::greater<Timestamp>>::type; nit: unusual indentation http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h@454 PS15, Line 454: // Transforms and writes deltas into 'deltas', a vector of "delta lists", each please add a description of the semantics of this method (what it's doing / why it should be called); only technical details have been provided so far in this comment maybe just: Collect all mutations relevant to a scan at timestamp ts. http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h@467 PS15, Line 467: continue break? http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h@750 PS15, Line 750: // Runs a fuzz test by generating a fairly random DeltaIterator, using it to Clarify what component is being tested here http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h@772 PS15, Line 772: inclusive snapshot what does this mean? the prng->Uniform() logic below treats .second as exclusive http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h@810 PS15, Line 810: - 1 nit: no need for this -1 because Random::Uniform() returns values from [0 .. n-1] http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.cc File src/kudu/tablet/tablet-test-util.cc: http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.cc@26 PS15, Line 26: to_include > ts; nit: maybe it's just me, but I usually think of this as ts < to_include, because to_include is sort of fixed, but obviously it's the same thing so feel free to ignore this http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.cc@32 PS15, Line 32: to_include <= ts same here, I usually think of this as ts >= to_include -- To view, visit http://gerrit.cloudera.org:8080/11140 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I539ff0f2055cf398a42efb824238188230e25516 Gerrit-Change-Number: 11140 Gerrit-PatchSet: 15 Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Mon, 29 Oct 2018 19:06:12 +0000 Gerrit-HasComments: Yes
