Will Berkeley 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 4: (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? This function prints progress messages like 0/1 replicas remaining (20B from disk, 10 rows summed) and that wrecks JSON formatting. Originally, the function itself checked IsNonJSONFormat, but since it got moved into another file the IsNonJSONFormat method is not defined, and it depends on a flag value I didn't want to leak into the file, so I altered the function to only print when it is supplied a non-null ostream* to print to. The "other half" is obscured in the diff because it's part of the new file ksck_checksum.cc, but there is a comment explaining this there, 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: et to use this special timest > "the current time that should"? Or maybe "The timestamp that should be used I rephrased to be more similar to the description of the flag that relates to this constant. Hopefully, it's clearer. http://gerrit.cloudera.org:8080/#/c/11488/3/src/kudu/tools/ksck_checksum.h@53 PS3, Line 53: ons(MonoDelta timeout, : int scan_concurrency, : bool use_snapshot, > nit: spacing Done 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 Done. Also fixed capitalization and grammar. 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 TabletServer Done http://gerrit.cloudera.org:8080/#/c/11488/3/src/kudu/tools/ksck_checksum.cc@73 PS3, Line 73: elta_rows); : > nit: let's keep the {} vs { Done http://gerrit.cloudera.org:8080/#/c/11488/3/src/kudu/tools/ksck_checksum.cc@127 PS3, Line 127: queue_(st > nit: string here and elsewhere Done -- 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: 4 Gerrit-Owner: Will Berkeley <[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: Fri, 21 Sep 2018 14:25:20 +0000 Gerrit-HasComments: Yes
