David Ribeiro Alves has posted comments on this change. Change subject: Add snapshot scans to fuzz-itest ......................................................................
Patch Set 10: (10 comments) http://gerrit.cloudera.org:8080/#/c/4996/10//COMMIT_MSG Commit Message: PS10, Line 12: operations > nit: comma after 'operations' Done Line 21: passes with it. > did you loop it a bunch, particularly in an ASAN build? yeah, unflaked fuzz-itest before this patch and then ran 500 loops of it. no failures. http://gerrit.cloudera.org:8080/#/c/4996/10/src/kudu/integration-tests/fuzz-itest.cc File src/kudu/integration-tests/fuzz-itest.cc: Line 80: using client::KuduInsert; > warning: using decl 'KuduInsert' is unused [misc-unused-using-decls] Done 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 Done PS10, Line 365: found > std::move this? Done Line 372: CHECK(false) << final_error; > LOG(FATAL)? Done 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: we need to keep removing the smallest timestamp so we'd have to add complexity to scan the set anyway. mind if I leave it like this? 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 th yeah, no need to sort, apparently. removed 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 yo yeah, since there is no need to sort i removed these. http://gerrit.cloudera.org:8080/#/c/4996/10/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: Line 1760: } else if (tmp_snap_timestamp > max_allowed_ts) { > warning: don't use else after return [readability-else-after-return] Done -- 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: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
