Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11029 )
Change subject: deltamemstore: support iteration with snap_to_exclude ...................................................................... Patch Set 9: Code-Review+1 (7 comments) looks good to me http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc File src/kudu/tablet/deltamemstore-test.cc: http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@142 PS9, Line 142: * nit: move the asterisk next to the type http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@579 PS9, Line 579: TEST_F(TestDeltaMemStore, TestScanSnapToExclude) { Mind adding a comment describing this test? Maybe something like: Test to ensure that scanning between two timestamps returns the difference between the two timestamps. http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@585 PS9, Line 585: // Sequence of operations: nit: Consider moving this part of the code down below the definition of ApplyAndCheck() so that this explanatory comment and setup code can easily fit on the same screen as the main assertions at the bottom of this test. http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@591 PS9, Line 591: snaps.emplace_back(mvcc_); nit: to make this easier to verify by inspection, mind adding // snaps[0] to the end of this line? and similar for other snapshots taken on the nearby lines below http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@592 PS9, Line 592: 0 nit: mind adding a const rowid_t kRowId = 0 up above so we can use it throughout this test? http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@600 PS9, Line 600: auto ApplyAndCheck = [&](const MvccSnapshot& exclude, Please add a doc comment along these lines to this lambda: // Apply mutations according to the specified time range to row 0 and assert // that the specified expected value and liveness of the row are satisfied. http://gerrit.cloudera.org:8080/#/c/11029/9/src/kudu/tablet/deltamemstore-test.cc@633 PS9, Line 633: false nit: mind changing this to /*is_row_live=*/ false ? Or maybe use an enum but I think the comment is less hassle. Similar below -- To view, visit http://gerrit.cloudera.org:8080/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 9 Gerrit-Owner: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Mon, 27 Aug 2018 18:04:07 +0000 Gerrit-HasComments: Yes
