Todd Lipcon has posted comments on this change. Change subject: Add snapshot scans to fuzz-itest ......................................................................
Patch Set 10: (8 comments) http://gerrit.cloudera.org:8080/#/c/4996/10//COMMIT_MSG Commit Message: PS10, Line 12: operations nit: comma after 'operations' Line 21: passes with it. did you loop it a bunch, particularly in an ASAN build? http://gerrit.cloudera.org:8080/#/c/4996/10/src/kudu/integration-tests/fuzz-itest.cc File src/kudu/integration-tests/fuzz-itest.cc: PS10, Line 289: int timestamp, : vector<ExpectedKeyValueRow> found, : list<string>* errors can you doc these parameters? not obvious from the names what's going on PS10, Line 365: found std::move this? Line 372: CHECK(false) << final_error; LOG(FATAL)? PS10, Line 529: // Sort the scan timestamps in reverse order so that we can keep popping from the back and remove : // duplicates. : sort(timestamps_to_scan.begin(), timestamps_to_scan.end(), std::greater<int>()); : timestamps_to_scan.erase(unique(timestamps_to_scan.begin(), : timestamps_to_scan.end()), : timestamps_to_scan.end() ); why not just use a std::set? this stuff isn't perf-critical so I think std::set and a ContainsKey() check is easier to read Line 576: sort(sorted_cur_val.begin(), sorted_cur_val.end()); not quite sure what the sort is accomplishing here. Is it to move all of the boost::none values to the front of the list? aside from that, aren't all of the rows already in sorted order because the indexes in this vector are the keys themselves? std::erase_if might be more obvious what's going on http://gerrit.cloudera.org:8080/#/c/4996/10/src/kudu/tablet/key_value_test_schema.h File src/kudu/tablet/key_value_test_schema.h: PS10, Line 46: bool operator < (const ExpectedKeyValueRow& other) const { : return (key < other.key); : } : : bool operator > (const ExpectedKeyValueRow& other) const { : return (key > other.key); : } : : a bit weird that these are not consistent with operator== above. I think you're just using this for the std::sort() call in the new test - could you just pass a lambda to std::sort that does the comparison you're looking for? -- To view, visit http://gerrit.cloudera.org:8080/4996 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
