Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11488 )
Change subject: [tools] ksck checksums: Factor out of main ksck code ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/11488/3/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/11488/3/src/kudu/tools/ksck.cc@638 PS3, Line 638: IsNonJSONFormat() ? out_ : nullptr); Why this change? http://gerrit.cloudera.org:8080/#/c/11488/3/src/kudu/tools/ksck_checksum.h File src/kudu/tools/ksck_checksum.h: http://gerrit.cloudera.org:8080/#/c/11488/3/src/kudu/tools/ksck_checksum.h@47 PS3, Line 47: that the current time should "the current time that should"? Or maybe "The timestamp that should be used for a checksum snapshot."? I suspect "current' is important, but I'm not sure why. http://gerrit.cloudera.org:8080/#/c/11488/3/src/kudu/tools/ksck_checksum.h@53 PS3, Line 53: int scan_concurrency, : bool use_snapshot, : uint64_t snapshot_timestamp); nit: spacing http://gerrit.cloudera.org:8080/#/c/11488/3/src/kudu/tools/ksck_checksum.cc File src/kudu/tools/ksck_checksum.cc: http://gerrit.cloudera.org:8080/#/c/11488/3/src/kudu/tools/ksck_checksum.cc@40 PS3, Line 40: "Maximum total seconds to wait for a checksum scan to complete " : "before timing out."); : DEFINE_int32(checksum_scan_concurrency, 4, : "Number of concurrent checksum scans to execute per tablet server."); : DEFINE_bool(checksum_snapshot, true, "Should the checksum scanner use a snapshot scan"); : DEFINE_uint64(checksum_snapshot_timestamp, : kudu::tools::KsckChecksumOptions::kCurrentTimestamp, : "timestamp to use for snapshot checksum scans, defaults to 0, which " : "uses the current timestamp of a tablet server involved in the scan"); nit: keep the punctuation, or lack thereof, consistent http://gerrit.cloudera.org:8080/#/c/11488/3/src/kudu/tools/ksck_checksum.cc@70 PS3, Line 70: nit: remove two spaces from all the ChecksumResultReporter and TabletServerChecksumCallbacks functions http://gerrit.cloudera.org:8080/#/c/11488/3/src/kudu/tools/ksck_checksum.cc@73 PS3, Line 73: { : } nit: let's keep the {} vs { } consistent (above too). http://gerrit.cloudera.org:8080/#/c/11488/3/src/kudu/tools/ksck_checksum.cc@127 PS3, Line 127: std::string nit: string here and elsewhere -- To view, visit http://gerrit.cloudera.org:8080/11488 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4bb1f51af22ab0c6c20b9426dbb62ea48413ed5b Gerrit-Change-Number: 11488 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley <wdberke...@gmail.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: Fri, 21 Sep 2018 07:43:14 +0000 Gerrit-HasComments: Yes
