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 8: (8 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 w The "of" is obligatory for the correct meaning, which is that ksck checksum code is being relocated from the main ksck code to somewhere else. Without the "of" the meaning (of "factor out main ksck code") is that ksck code is being relocated somewhere from wherever it is. http://gerrit.cloudera.org:8080/#/c/11488/6//COMMIT_MSG@10 PS6, Line 10: out > nit: drop? Done 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 decla Done 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 It's not, it's just relocated from elsewhere in the overall ksck code. 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 Done 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. Done 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() - I'm going to skip doing this just because this would make me have to resolve the conflict on rebase when this bit changes a little. Feel free to re-nit the KUDU-2179 patch with this. 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 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: 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 06:14:06 +0000 Gerrit-HasComments: Yes
