Alexey Serbin 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 6: (4 comments) A couple of nits, otherwise LGTM. http://gerrit.cloudera.org:8080/#/c/11488/6/src/kudu/tools/ksck_checksum.h File src/kudu/tools/ksck_checksum.h: http://gerrit.cloudera.org:8080/#/c/11488/6/src/kudu/tools/ksck_checksum.h@46 PS6, Line 46: public: nit: is it necessary due to code convention? If not, I think it's better to drop this. http://gerrit.cloudera.org:8080/#/c/11488/6/src/kudu/tools/ksck_checksum.cc File src/kudu/tools/ksck_checksum.cc: http://gerrit.cloudera.org:8080/#/c/11488/6/src/kudu/tools/ksck_checksum.cc@102 PS6, Line 102: int It's better to use 'auto' here: ToMilliseconds() is int64_t. http://gerrit.cloudera.org:8080/#/c/11488/6/src/kudu/tools/ksck_checksum.cc@108 PS6, Line 108: MonoTime::Now() MonoTime::Now() has been captured already at line 101: maybe reuse it here? http://gerrit.cloudera.org:8080/#/c/11488/6/src/kudu/tools/ksck_checksum.cc@109 PS6, Line 109: << "Checksum " << status << run_time_sec << "s: " : << responses_.count() << "/" << expected_count_ << " replicas remaining (" : << HumanReadableNumBytes::ToString(disk_bytes_summed_.Load()) << " from disk, " : << HumanReadableInt::ToString(rows_summed_.Load()) << " rows summed)" : << endl; > nit: mind writing this with `Substitute()`? +1 -- 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: 6 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 20:48:13 +0000 Gerrit-HasComments: Yes
