Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11498 )
Change subject: [tools] ksck checksums: Add KsckChecksummer class ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.h File src/kudu/tools/ksck_checksum.h: http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.h@57 PS3, Line 57: KsckChecksumOptions(); : : KsckChecksumOptions(std::vector<std::string> table_filters, : std::vector<std::string> tablet_id_filters); : : KsckChecksumOptions(MonoDelta timeout, : int scan_concurrency, : bool use_snapshot, : uint64_t snapshot_timestamp); : : KsckChecksumOptions(MonoDelta timeout, : int scan_concurrency, : bool use_snapshot, : uint64_t snapshot_timestamp, : std::vector<std::string> table_filters, : std::vector<std::string> tablet_id_filters); Did you consider adding KsckCheckOptionsBuilder instead? http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.h@186 PS3, Line 186: class KsckChecksummer { nit: do you mind re-order the methods in the ksck_checksum.cc file to match their declaration order? http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.h@188 PS3, Line 188: explicit KsckChecksummer(KsckCluster* cluster); docs nit: does it make sense to add a note explaining that 'cluster' should be alive for the whole lifetime of an instance of this class? http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.h@216 PS3, Line 216: // NOTE: Even if this function returns a bad Status, 'table_checksum_map' : // and 'num_results' will still be populated using whatever results are : // available. Is that implementation detail is exposed because you want somebody to rely on this or that's more about implementation optimization? http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.cc File src/kudu/tools/ksck_checksum.cc: http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.cc@186 PS3, Line 186: spacing nit: this supposed to be 4 spaces http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.cc@186 PS3, Line 186: cluster_(cluster) { : CHECK(cluster); nit: it might be : cluster_(CHECK_NOTNULL(cluster)) { } http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.cc@267 PS3, Line 267: string nit: would 'auto' work here as well? -- To view, visit http://gerrit.cloudera.org:8080/11498 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2016936eaa26fd6b499783e7d5d8f404816b37fa Gerrit-Change-Number: 11498 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Mon, 01 Oct 2018 21:25:18 +0000 Gerrit-HasComments: Yes
