Adar Dembo 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) 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 Done 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 / I don't think your suggestion is adding more information than already exists. "Collect all mutations" is implied by the method name. And "relevant to a scan at timestamp ts" is already mentioned in the second paragraph, in "Deltas not relevant to 'ts' are skipped". http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h@467 PS15, Line 467: continue > break? Done 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 Done 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 excl Yeah this wasn't clear. This is referring to how when i == kNumScans, the snapshot we use is "totally inclusive", which is an ambiguous way of saying "a snapshot for which all deltas are relevant". 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 . Done 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, be Done 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 Done -- 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: Tue, 30 Oct 2018 04:00:23 +0000 Gerrit-HasComments: Yes
