Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10151 )
Change subject: [tools] ksck improvements [6/n]: Refactor printing ...................................................................... Patch Set 9: (16 comments) Most entirely nits, I think this would have been more easily reviewable with some updates to the commit message. http://gerrit.cloudera.org:8080/#/c/10151/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10151/5//COMMIT_MSG@17 PS5, Line 17: - Removes the --consensus flag. ksck is far less useful without it and : it doesn't seem like having it brings any benefit. > I think it's not worth separating that out if the separation would bring to I second Alexey's concern with putting this into a different patch. I also (less strongly) second his conclusion that it's not worth it to separate it out. http://gerrit.cloudera.org:8080/#/c/10151/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10151/9//COMMIT_MSG@7 PS9, Line 7: Refactor printing nit: Maybe this started out being the case, but I think this patch delves a bit deeper into not-just-printing stuff. It's more apt to call this something like "refactor result handling", which I think would make it clearer to readers what to expect in this patch. http://gerrit.cloudera.org:8080/#/c/10151/9//COMMIT_MSG@9 PS9, Line 9: It separates where : checks are done from where results are printed It'd be helpful at a glance to describe what this separation is. A stab at it: "The various results yielded for runs of the ksck tool are now encapsulated and collected in the new KsckResults class. This encapsulation will..." http://gerrit.cloudera.org:8080/#/c/10151/9//COMMIT_MSG@15 PS9, Line 15: running all ksck checks nit: could you flesh out what this means a little bit? Not having read through the patch, this could mean running the CLI tool is easier, the public programming interface is better, etc. http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck-test.cc@632 PS9, Line 632: "1 out of nit: spacing here and elsewhere with similar error messages above and below http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck-test.cc@674 PS9, Line 674: ASSERT_STR_CONTAINS nit: spacing here and elsewhere in similar summary messages http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck-test.cc@808 PS9, Line 808: ASSERT_TRUE nit: spacing here and in the below runtime error assertion http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck.h File src/kudu/tools/ksck.h: http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck.h@464 PS9, Line 464: the proper order, including checksum scans, : // if enabled. nit: is there some heuristic that specifies what the proper order is? http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck.cc@321 PS9, Line 321: CheckClusterRunning() and FetchTableAndTabletInfo nit: we should probably keep the parens consistent http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck.cc@637 PS9, Line 637: licaResultMap::value_type& r : : FindOrDie(checksums, tablet->id())) nit: spacing http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck_results.h File src/kudu/tools/ksck_results.h: http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck_results.h@105 PS9, Line 105: // A config must have a term, but the master does not disclose it. : boost::optional<int64_t> term; Making sure I understand: this field would be non-optional, but masters, which use KsckConsensusState, do not make it public. Is that strictly dependent on `type`? Are there checks that enforce this (e.g. CHECK(type == master || term) or something)? http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck_results.h@141 PS9, Line 141: .. nit: extra . http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck_results.h@246 PS9, Line 246: std::vector<Status> error_messages; How do we reconcile what checks correspond to what error messages? I guess the ordering is somewhat hard-coded, huh? http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck_results.cc File src/kudu/tools/ksck_results.cc: http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck_results.cc@99 PS9, Line 99: const KsckConsensusState& cstate, : const map<string, char> replica_labels, : DataTable* table) { nit: spacing http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/kudu-admin-test.cc@341 PS9, Line 341: EnableKudu1097AndDownTS nit: this seems easy enough to put into a separate patch http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/tool_action_tablet.cc File src/kudu/tools/tool_action_tablet.cc: http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/tool_action_tablet.cc@413 PS9, Line 413: // We retry since occasionally ksck returns bad status because of transient : // issues like leader elections. : RETURN_NOT_OK_PREPEND(WaitForCleanKsck(master_addresses, : tablet_id, : MonoDelta::FromSeconds(5)), nit: this seems like it should be a separate patch as well -- To view, visit http://gerrit.cloudera.org:8080/10151 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 Gerrit-Change-Number: 10151 Gerrit-PatchSet: 9 Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Fri, 27 Apr 2018 01:27:00 +0000 Gerrit-HasComments: Yes
