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 8: (9 comments) http://gerrit.cloudera.org:8080/#/c/11488/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11488/6//COMMIT_MSG@7 PS6, Line 7: of nit: maybe, drop this? From the other side, I might be just not familiar with the usage of 'factor out' as a noun. http://gerrit.cloudera.org:8080/#/c/11488/6//COMMIT_MSG@10 PS6, Line 10: out nit: drop? http://gerrit.cloudera.org:8080/#/c/11488/6/src/kudu/integration-tests/cluster_verifier.cc File src/kudu/integration-tests/cluster_verifier.cc: http://gerrit.cloudera.org:8080/#/c/11488/6/src/kudu/integration-tests/cluster_verifier.cc@55 PS6, Line 55: checksum_options_(tools::KsckChecksumOptions()), nit: maybe, drop this line and reorder the fields of the class in the declaration? It's just a tiny nit, so feel free to ignore. http://gerrit.cloudera.org:8080/#/c/11488/6/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/11488/6/src/kudu/tools/ksck.cc@637 PS6, Line 637: // Don't ruin JSON output by printing progress updates. : auto* out_for_progress_updates = IsNonJSONFormat() ? out_ : nullptr; : bool timed_out = !reporter->WaitFor(options.timeout, out_for_progress_updates); This is a small, but functional update, isn't it? If so, consider updating the commit message correspondingly. http://gerrit.cloudera.org:8080/#/c/11488/8/src/kudu/tools/ksck_checksum.h File src/kudu/tools/ksck_checksum.h: http://gerrit.cloudera.org:8080/#/c/11488/8/src/kudu/tools/ksck_checksum.h@46 PS8, Line 46: // A checksum initially set to use this special timestamp will actually use : // a snapshot timestamp selected by some tablet server involved in the scan. nit: this looks a bit complicated to me. How about rephrasing this as A checksum with this special timestamp will use a timestamp selected by one of tablet servers performing the snapshot scan. http://gerrit.cloudera.org:8080/#/c/11488/8/src/kudu/tools/ksck_checksum.h@144 PS8, Line 144: scoped_refptr<ChecksumResultReporter> reporter, : std::shared_ptr<KsckTabletServer> tablet_server, : SharedTabletQueue queue, : std::string tablet_id, : nit: here the indent is supposed to be additional 4 spaces, I think. 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@108 PS6, Line 108: For( > Note that on L105 we WaitFor'd for up to 5000ms, so L101's Now() is probabl Woops, indeed. http://gerrit.cloudera.org:8080/#/c/11488/8/src/kudu/tools/ksck_checksum.cc File src/kudu/tools/ksck_checksum.cc: http://gerrit.cloudera.org:8080/#/c/11488/8/src/kudu/tools/ksck_checksum.cc@116 PS8, Line 116: run_time_sec, nit: maybe, use HumanReadableElapsedTime::ToShortString((MonoTime::Now() - start).ToSeconds()) instead? You could drop the run_time_sec variable and extra "s" in the message format: "...$0 $1s..." --> "...$0 $1..." http://gerrit.cloudera.org:8080/#/c/11488/8/src/kudu/tools/ksck_checksum.cc@132 PS8, Line 132: : nit: move this into the next line -- 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: 8 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: Tue, 02 Oct 2018 05:37:00 +0000 Gerrit-HasComments: Yes
