Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11498 )
Change subject: [tools] ksck checksums: Add KsckChecksummer class ...................................................................... Patch Set 2: (11 comments) http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck.cc@a637 PS2, Line 637: > nit: Maybe keep this around as a note? It's probably not too difficult to r Done http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck.cc@425 PS2, Line 425: table_filters_for_checksum_opts, : tablet_id_filters_for_checksum_opts > We should std::move these Done 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? No, because this isn't a client-facing API, and these 4 cover all the actual cases. 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 The method changes a fair amount in a follow-up, enough so that a rebase is tedious and error-prone. Is it ok to leave ChecksumData out of place for this patch and move it in the follow-up? 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 Done 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 It's not an implementation detail. It's an important part of how the class works: even if we checksum 10/20 replicas before we run out of time, the 10 results should still be reported. This comment is just to make this extra clear. http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck_checksum.cc File src/kudu/tools/ksck_checksum.cc: http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck_checksum.cc@188 PS2, Line 188: const KsckCluster& cluster, > `cluster_` is already a private member. What's the thought process behind m I feel like making the method static means how it affects state is more explicit, while keeping it tied to the class it's relevant to. Making it static says both "this doesn't affect the state of an instance" and "this doesn't involve the state of the instance". Instead, it's just a related function, like these two which just do some data shuffling and populating into an out param. I'm not married to the concept though. I can make them const methods if you like. http://gerrit.cloudera.org:8080/#/c/11498/2/src/kudu/tools/ksck_checksum.cc@345 PS2, Line 345: timestamp of the : // some > nit: extra word Done 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: St > spacing nit: this supposed to be 4 spaces Done http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.cc@186 PS3, Line 186: us KsckChecksummer::BuildTabletTableMap( : const KsckChe > nit: it might be Done http://gerrit.cloudera.org:8080/#/c/11498/3/src/kudu/tools/ksck_checksum.cc@267 PS3, Line 267: > nit: would 'auto' work here as well? Done -- 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: 2 Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Mon, 01 Oct 2018 21:54:57 +0000 Gerrit-HasComments: Yes
