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 6: (7 comments) 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 t Done http://gerrit.cloudera.org:8080/#/c/11488/6/src/kudu/tools/ksck_checksum.h@78 PS6, Line 78: disk_bytes_summed > `delta_disk_bytes_summed`, or rename the param to be `disk_bytes_summed` to It's just a typo in the comment. Done. 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. Done http://gerrit.cloudera.org:8080/#/c/11488/6/src/kudu/tools/ksck_checksum.cc@106 PS6, Line 106: string status = done ? "finished in " : "running for "; > nit: could be moved into the `if` Done 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? Note that on L105 we WaitFor'd for up to 5000ms, so L101's Now() is probably pretty far from L108's Now(). 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()`? Done 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; > +1 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: 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 21:55:07 +0000 Gerrit-HasComments: Yes
